Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d2351c0f-0bfe-9422-f6f3-f0a0db58c729@sangfor.com.cn>
Date:   Wed, 3 May 2023 22:00:49 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
From:   Ding Hui <dinghui@sangfor.com.cn>
Subject: Re: [PATCH net v4 2/2] iavf: Fix out-of-bounds when setting channels
 on remove
To:     Leon Romanovsky <leon@kernel.org>
Cc:     dinghui@sangfor.com.cn, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com,
        intel-wired-lan@lists.osuosl.org, jesse.brandeburg@intel.com,
        anthony.l.nguyen@intel.com, keescook@chromium.org,
        grzegorzx.szczurek@intel.com, mateusz.palczewski@intel.com,
        mitch.a.williams@intel.com, gregory.v.rose@intel.com,
        jeffrey.t.kirsher@intel.com, michal.kubiak@intel.com,
        simon.horman@corigine.com, madhu.chittim@intel.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org, pengdonglin@sangfor.com.cn,
        huangcun@sangfor.com.cn
References: <20230503031541.27855-1-dinghui@sangfor.com.cn>
 <20230503031541.27855-3-dinghui@sangfor.com.cn>
 <20230503082458.GH525452@unreal>
Content-Language: en-US
In-Reply-To: <20230503082458.GH525452@unreal>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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