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: <8c243d68-583f-aeea-3d8f-e608746dd9e7@sangfor.com.cn>
Date:   Mon, 8 May 2023 20:34:43 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH net v4 2/2] iavf: Fix out-of-bounds when setting channels
 on remove
Content-Language: en-US
To:     Leon Romanovsky <leon@kernel.org>,
        "Chittim, Madhu" <madhu.chittim@intel.com>
Cc:     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,
        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>
 <d2351c0f-0bfe-9422-f6f3-f0a0db58c729@sangfor.com.cn>
 <20230503162932.GN525452@unreal>
 <941ad3cc-22d6-3459-dfbc-36bc47a8a22a@intel.com>
 <20230504075709.GS525452@unreal>
From:   Ding Hui <dinghui@sangfor.com.cn>
In-Reply-To: <20230504075709.GS525452@unreal>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2023/5/4 15:57, Leon Romanovsky wrote:
> On Wed, May 03, 2023 at 12:22:00PM -0700, Chittim, Madhu wrote:
>>
>>
>> On 5/3/2023 9:29 AM, Leon Romanovsky wrote:
>>> 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]
>>>
>>>
>>
>> __IAVF_IN_REMOVE_TASK is being set only in iavf_remove() and the above
>> change is ok in terms of coming out of setting channels early enough while
>> remove is in progress.
> 
> It is not, __IAVF_IN_REMOVE_TASK, set bit can be changed any time during
> iavf_set_channels() and if it is not, I would expect test_bit(..) placed
> at the beginning of iavf_set_channels() or even earlier.
> 

Since we have a little dispute on __IAVF_IN_REMOVE_TASK, I'll remove the
test_bit() in v5, and remove Reviewed-by: tags of 2/2 to review again.

-- 
Thanks,
- Ding Hui