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

[netdev-bot+sashiko@kernel.org]

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

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928230429.495442-1-anthony.l.nguyen%40intel.com