Re: [Intel-wired-lan] [PATCH net v4 2/2] iavf: Fix out-of-bounds when setting channels on remove

From: Leon Romanovsky <leon@kernel.org>
To: Ding Hui <dinghui@sangfor.com.cn>
Cc: pengdonglin@sangfor.com.cn, keescook@chromium.org,
	gregory.v.rose@intel.com, anthony.l.nguyen@intel.com,
	mitch.a.williams@intel.com, jesse.brandeburg@intel.com,
	huangcun@sangfor.com.cn, linux-kernel@vger.kernel.org,
	grzegorzx.szczurek@intel.com, edumazet@google.com,
	michal.kubiak@intel.com, intel-wired-lan@lists.osuosl.org,
	jeffrey.t.kirsher@intel.com, simon.horman@corigine.com,
	kuba@kernel.org, netdev@vger.kernel.org, pabeni@redhat.com,
	davem@davemloft.net, linux-hardening@vger.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH net v4 2/2] iavf: Fix out-of-bounds when setting channels on remove
Date: Wed, 3 May 2023 19:29:32 +0300	[thread overview]
Message-ID: <20230503162932.GN525452@unreal> (raw)
In-Reply-To: <d2351c0f-0bfe-9422-f6f3-f0a0db58c729@sangfor.com.cn>

On Wed, May 03, 2023 at 10:00:49PM +0800, Ding Hui wrote:
> On 2023/5/3 4:24 下午, Leon Romanovsky wrote:
> > On Wed, May 03, 2023 at 11:15:41AM +0800, Ding Hui wrote:
> 
> > > 
> > > If we detected removing is in processing, we can avoid unnecessary
> > > waiting and return error faster.
> > > 
> > > On the other hand in timeout handling, we should keep the original
> > > num_active_queues and reset num_req_queues to 0.
> > > 
> > > Fixes: 4e5e6b5d9d13 ("iavf: Fix return of set the new channel count")
> > > Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
> > > Cc: Donglin Peng <pengdonglin@sangfor.com.cn>
> > > Cc: Huang Cun <huangcun@sangfor.com.cn>
> > > Reviewed-by: Simon Horman <simon.horman@corigine.com>
> > > Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
> > > ---
> > > v3 to v4:
> > >    - nothing changed
> > > 
> > > v2 to v3:
> > >    - fix review tag
> > > 
> > > v1 to v2:
> > >    - add reproduction script
> > > 
> > > ---
> > >   drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > > index 6f171d1d85b7..d8a3c0cfedd0 100644
> > > --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > > @@ -1857,13 +1857,15 @@ static int iavf_set_channels(struct net_device *netdev,
> > >   	/* wait for the reset is done */
> > >   	for (i = 0; i < IAVF_RESET_WAIT_COMPLETE_COUNT; i++) {
> > >   		msleep(IAVF_RESET_WAIT_MS);
> > > +		if (test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
> > > +			return -EOPNOTSUPP;
> > 
> > This makes no sense without locking as change to __IAVF_IN_REMOVE_TASK
> > can happen any time.
> > 
> 
> The state doesn't need to be that precise here, it is optimized only for
> the fast path. During the lifecycle of the adapter, the __IAVF_IN_REMOVE_TASK
> state will only be set and not cleared.
> 
> If we didn't detect the "removing" state, we also can fallback to timeout
> handling.
> 
> So I don't think the locking is necessary here, what do the maintainers
> at Intel think?

I'm not Intel maintainer, but your change, explanation and the following
line from your commit message aren't really aligned.

[ 3510.400799] ==================================================================
[ 3510.400820] BUG: KASAN: slab-out-of-bounds in iavf_free_all_tx_resources+0x156/0x160 [iavf]

> 
> > Thanks
> > 
> > >   		if (adapter->flags & IAVF_FLAG_RESET_PENDING)
> > >   			continue;
> > >   		break;
> > >   	}
> > >   	if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
> > >   		adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
> > > -		adapter->num_active_queues = num_req;
> > > +		adapter->num_req_queues = 0;
> > >   		return -EOPNOTSUPP;
> > >   	}
> > > -- 
> > > 2.17.1
> > > 
> > > 
> > 
> 
> -- 
> Thanks,
> -dinghui
> 
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan