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: <941ad3cc-22d6-3459-dfbc-36bc47a8a22a@intel.com>
Date:   Wed, 3 May 2023 12:22:00 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
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>,
        Ding Hui <dinghui@sangfor.com.cn>
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>
From:   "Chittim, Madhu" <madhu.chittim@intel.com>
In-Reply-To: <20230503162932.GN525452@unreal>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WnZzMkFlRU5mM1Y4RUhPbEJ5eDEyWExXeUJ2V2RDR29UQlo5Nlk5NUp4Rkdn?=
 =?utf-8?B?bC9MVng5SnVNWTM3cDlmRERUUjRoZE1sT1YxY1hHVGlDdlRIV2g2SnZrak5I?=
 =?utf-8?B?SFl4NTBXb25lSGtmMlR5eEpDYStibUZGOGk5VUNQQittVGNDN0hHMVArS25G?=
 =?utf-8?B?d2J4cjdTRXdVWTlDTm0vQjRkVXpFRk4zQUhQOWZ0cTNVeTBTeG80VVliamNs?=
 =?utf-8?B?S1BFMFZmdTJpNDBYU0NCaU5PRWsrTXlQeUpjRTgwZFptM3pYQjN6dFI3RXFl?=
 =?utf-8?B?MVcvTitIVUk5U3RqY3RpeStsZTNzTCs2anNqa3NtNkxJNnpPd1JFOElUOTNN?=
 =?utf-8?B?a1hycys2c3FEbmtuTTVWL3U0VDU3T1VmYkdlLzlwcllrRVBFTndxUC9wOGgz?=
 =?utf-8?B?dWRkL2FKVDdwUFRwMDRreUpHSXVSdnRwMktucnJZdTZFYlhRaU9RTEpXRUZp?=
 =?utf-8?B?bUNOMkF5YUVMRzN4bnpqUjhGd3lMcHZPZDNtMytjc2NYemFSb0srM1QzS00z?=
 =?utf-8?B?R0tsVitUbHFoSENYUnFSQ2ovQWR1cGdXaUV6SzFEUy9oS2FwQmE0c0FxVUEr?=
 =?utf-8?B?eERjeHRrYy8yTkc0STg3eGhLRjIvNEVzdUVCdGxXMGI4UFF1c053Wk1xTndM?=
 =?utf-8?B?Z3VKTXFocWpNdDhMY2RVM2RIUWQ2N3VoT3gwcnBQNGRxb2dpOVZaZnNXUTAy?=
 =?utf-8?B?WTR6THJ5c1pMRlZOamtHL01rNDVkM2NINEx6WDJqZzVGNksrbG90QTkwVytk?=
 =?utf-8?B?MEVqTC9leW5lN0RFTThDS3Y0dlVQQzV1eGNiOXZmSndyR1czSm90RzlHazh6?=
 =?utf-8?B?Lyt2SVdTUk8xaG9VRDhObmhtM3I2eXRRTTlIdDVHdG1JUzAxOEhYUCs0bXQz?=
 =?utf-8?B?a1AxNWhhS29VcVo5eno5NUFIdkxwd01nQzhJbTJuY3RGbHl5ZG9lVDJVNE1E?=
 =?utf-8?B?dFlSMGVEZC92aVg4Ynh1R1lUN1FYK3RqaUt6S3dUdEt3cEZJTXJzRlZqOFZ4?=
 =?utf-8?B?ZXp0TFM3UlZESDRtUTdNTkZHNWo0bndtMTh1MW9TTjkvU0Iwa2V5MExuNlU3?=
 =?utf-8?B?Y2hPRzlzOG9saGRvaHg4VVhEN2NzcTdoWk5sRzBLSlF6VGpkeXF2SGRvcjNL?=
 =?utf-8?B?STk4b3V5cHZmRXlrUWhuQURoMlo3YWpEWHNuWmlGZzFaeHF4UnhNN1NILzJW?=
 =?utf-8?B?aTF6bXgvR29WdHc0dUFuWGk2T08xOHE1MmdIYkFEL2JOUEZwZFc5eStzZWIz?=
 =?utf-8?B?OWlPbFFsVDQzNzdmcTEzQy9KZE12RFBaeVJwQk1GNjFzSEg1RVg2V0gzS2pT?=
 =?utf-8?B?V2xGaG1zNlp3cGU0dlVISUdUakZ0d1BpdHNJTGNMT2hwbXJqNlpXUEltemky?=
 =?utf-8?B?dDZjcVR4NlZ2WmVKVGNFS20vR0xGS3ZJRFBjRG9FaWVmcmRmQnppTTJ4d1ov?=
 =?utf-8?B?MVdhRGRvbjRIMmJSSHI4WmlCUG4rRDlyaXZBVkd4c3BzM1lsNlBLVGdtZkQ0?=
 =?utf-8?B?ZlJQQXU2bEZsSmRGaUJzZ0lOWU40NmQxRDR6L1QwVThtRG1QTHIyb0JYSlN6?=
 =?utf-8?B?ckJnbHJac1d4VG5aS1F1QVlLTUFwVWszeFJmVWFpRDZURmgyekxhMXM5bXpu?=
 =?utf-8?B?L1VnQmU1TStaOUdYNnphbUxRY2JkS1dRMGRydDRuS0lSQkNYc0wwaDVCT1hG?=
 =?utf-8?B?ZWdXaHJJSW1FUXpyc1FTZEhhUnNDdFh1djBkOFAycmtXQXVKR01Zc3Bhc3Y1?=
 =?utf-8?B?UDJFWTljcHJrQ0NiUXFLVW5iUW5EZWVQSXpuSEZMNlhvY0NLRHpYeWJUY0NR?=
 =?utf-8?B?RDZST3c5WFBBa0ZsVjJ6LzdmR3JDUjFZWVlxZENtcWdadXY2ZTQ5NFR3ZCtj?=
 =?utf-8?B?MUp6TGZ2ZUxPQ0xWQmwwOElzVjljUWo2QVN0SVcxWTdIOXEwdTV5WTA1V0Yv?=
 =?utf-8?B?NHN2SU1nY3EyZG1vbWkyVkVPNGhrTmh3ZzROcDJtOGNNTFhMZWF5aTZ5NVFm?=
 =?utf-8?B?YTY5UC9Tcyt1ZHNXTUl4ZVh0dUdLSXBDSVNnUzI5aS8wUUN3bjM3cVNXNG5n?=
 =?utf-8?B?RFRQMHRXdlE0SGZORXhNNU1OSlBDbkh3czRwWVd2bkRpUEpUbG5YR3VpUWtP?=
 =?utf-8?B?U2NYTnlEK2Z0Tmt0VlpMdEJzSDdPUllvemdWRWx4Rkw1R1Y3d3VqRFM1WFFl?=
 =?utf-8?B?RXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ef8a49b8-c6c7-4b14-a224-08db4c0bad3e
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5042.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2023 19:22:03.9940
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rtZ9Va7/Kr9X0g0yD8XiLKdi7+z6YhuYsQlKY1cRiN3C/BRteb6+7Ul78QJL+t7OU5CJRpu6D0c64+sEi4/HdQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6197
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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.

Reviewed-by: madhu.chittim@intel.com

>>
>>> 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
>>
>>