Re: [PATCH net 1/6] idpf: fix possible race on remove during a reset

["Tantilov, Emil S" <emil.s.tantilov@intel.com>]

On 9/29/2026 5:58 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The patch removes the IDPF_REMOVE_IN_PROG early return from
>    idpf_stop(), which opens a double-teardown race between a userspace…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the patch
>    relies on the loop that contains it.
> - [High] This problem wasn't introduced by this patch, but the patch now
>    sends the remove-racing-reset case through it.
> 
> --
> 
> idpf: fix possible race on remove during a reset
> 
> This patch drops the IDPF_REMOVE_IN_PROG early return from idpf_stop(),
> so idpf_vport_stop() always runs on close. The goal is to let a reset
> that races with remove tear down NAPI in idpf_detach_and_close(), which
> runs under RTNL.
> 
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
>> index 827c795afcb62..2c148377540cd 100644
>> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
>> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
>> @@ -1033,12 +1033,8 @@ static void idpf_vport_stop(struct idpf_vport *vport, bool rtnl)
>>    */
>>   static int idpf_stop(struct net_device *netdev)
>>   {
>> -	struct idpf_netdev_priv *np = netdev_priv(netdev);
>>   	struct idpf_vport *vport;
>>   
>> -	if (test_bit(IDPF_REMOVE_IN_PROG, np->adapter->flags))
>> -		return 0;
>> -
> 
> [Severity: High]
> Without this check, can a userspace close that runs during idpf_remove()
> tear down the vport a second time?
> 
> The remove path is idpf_remove() -> idpf_vc_core_deinit() ->
> idpf_deinit_task() -> idpf_vport_dealloc() -> idpf_vport_stop(vport, true).
> It holds neither vport_ctrl_lock nor RTNL on entry. idpf_vport_stop() tests
> the UP bit before it takes RTNL and does not test it again afterwards:
> 
> idpf_vport_stop() {
> 	if (!test_bit(IDPF_VPORT_UP, np->state))
> 		return;
> 
> 	if (rtnl)
> 		rtnl_lock();
>      ...
> }
> 
> This is the interleaving in question:
> 
> CPU1 (rmmod)                         CPU2 (ip link set dev down)
>                                       __dev_close_many() holds RTNL
> idpf_vport_stop(vport, true)
>    IDPF_VPORT_UP is set
>    rtnl_lock() blocks
>                                       idpf_stop()
>                                         idpf_vport_stop(vport, false)
>                                           queues/vectors released
>                                           IDPF_VPORT_UP cleared
>                                       rtnl_unlock()
>    teardown continues on released state
> 
> idpf_vport_intr_rel() sets q_vectors to NULL but does not change
> num_q_vectors:
> 
> 	kfree(rsrc->q_vectors);
> 	rsrc->q_vectors = NULL;
> 
> On the second pass, idpf_vport_intr_dis_irq_all() does:
> 
> 	struct idpf_q_vector *q_vector = rsrc->q_vectors;
> 	...
> 	for (u16 q_idx = 0; q_idx < rsrc->num_q_vectors; q_idx++)
> 		writel(0, q_vector[q_idx].intr_reg.dyn_ctl);
> 
> Won't that dereference a NULL q_vector? The virtchnl message builders
> that walk the NULLed rsrc->txq_grps/rxq_grps may also crash before that
> point.
> 
> Before this patch, a close that reached idpf_stop() after remove had set
> IDPF_REMOVE_IN_PROG returned 0, so only one teardown ran.
> 
> Would it help to re-check IDPF_VPORT_UP in idpf_vport_stop() after
> rtnl_lock() is taken, or to hold vport_ctrl_lock around the stop in the
> remove path?
> 
>>   	idpf_vport_ctrl_lock(netdev);
>>   	vport = idpf_netdev_to_vport(netdev);
>>

This is a valid hit. I will post v2 to address it. For the pre-existing 
below, I will review and post fixes as needed.

Thanks,
Emil

> 
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch. It
> matters here because this fix now depends on the loop in
> idpf_detach_and_close():
> 
> 	for (int i = 0; i < max_vports; i++) {
> 		struct net_device *netdev = adapter->netdevs[i];
> 		...
> 		if (!netif_device_present(netdev))
> 			continue;
> 
> idpf_vc_core_init() sizes adapter->netdevs by caps.max_vports. However,
> idpf_init_task() only creates netdevs up to default_num_vports. If the
> device reports max_vports > default_num_vports, can the first hard reset
> pass a NULL netdev to netif_device_present()?
> 
> idpf_vc_event_task() -> idpf_init_hard_reset() -> idpf_detach_and_close()
>    -> netif_device_present(NULL)
> 
> The other max_vports-bounded loops in the driver check for empty slots.
> idpf_remove() does "if (!adapter->netdevs[i]) continue;", idpf_init_task()
> checks !netdev, and idpf_decfg_netdev() stores NULL into the slot on
> purpose.
> 
> It is not clear which shipping firmware reports max_vports larger than
> default_num_vports. virtchnl2 defines them as separate fields, though.
> This has been present since 2e281e1155fc, the commit named in Fixes:.
> 
> [Severity: High]
> This is also a pre-existing issue and was not introduced by this patch.
> However, the remove-racing-reset case now goes through it as well.
> 
> On a software-initiated function reset, the transaction manager is shut
> down before the close path runs:
> 
> idpf_vc_event_task() {
>      ...
> func_reset:
> 	if (adapter->xnm)
> 		libie_ctlq_xn_shutdown(adapter->xnm);
> drv_load:
> 	set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
> 	idpf_init_hard_reset(adapter);
>      ...
> }
> 
> After that, libie_ctlq_xn_pop_free() refuses new transactions:
> 
> 	if (unlikely(xnm->shutdown))
> 		return NULL;
> 
> The call chain is idpf_init_hard_reset() -> idpf_detach_and_close() ->
> dev_close() -> idpf_stop() -> idpf_vport_stop(). The DISABLE_VPORT and
> DISABLE_QUEUES messages fail silently because their return values are
> ignored. Then the rings and buffers are freed:
> 
> 	idpf_vport_intr_deinit(vport, rsrc);
> 	idpf_xdp_rxq_info_deinit_all(rsrc);
> 	idpf_vport_queues_rel(vport, rsrc);
> 	idpf_vport_intr_rel(rsrc);
> 
> reg_ops->trigger_reset() is only called later, after
> idpf_vc_core_deinit(). Can the still-enabled Rx queues DMA into the freed
> descriptor rings or buffers during that window?
> 
> On a VF, idpf_vf_trigger_reset() skips the reset entirely during remove:
> 
> 	if (trig_cause == IDPF_HR_FUNC_RESET &&
> 	    !test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
> 		idpf_send_vf_reset_msg(adapter);
> 
> In that case, no device reset follows the free at all.
> 
> Before this patch, the remove-racing-reset case skipped idpf_vport_stop()
> in idpf_stop(). The rings leaked, but they were not freed while still in
> use.
> 
> Would it help to shut down the transaction manager only after
> idpf_detach_and_close() has disabled the queues? Another option is to
> keep the DMA memory until the device has been reset.
>