lkml.org 
[lkml]   [2023]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2023-05-03 16:02    [W:1.052 / U:1.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site