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

[Ding Hui]

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@xxxxxxxxxxxxxx>
> > Cc: Donglin Peng <pengdonglin@xxxxxxxxxxxxxx>
> > Cc: Huang Cun <huangcun@xxxxxxxxxxxxxx>
> > Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>
> > Reviewed-by: Michal Kubiak <michal.kubiak@xxxxxxxxx>
> > ---
> > 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?

> 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