Messages in this thread | | | Date | Wed, 3 May 2023 22:00:49 +0800 | From | Ding Hui <> | Subject | Re: [PATCH net v4 2/2] iavf: Fix out-of-bounds when setting channels on remove |
| |
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?
> 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
| |