[RFC net] netdev-genl: skip DEL_NTF on NETDEV_UNREGISTER rebroadcast

[RFC net] netdev-genl: skip DEL_NTF on NETDEV_UNREGISTER rebroadcast

[Xiaoguang Wang]

While netdev_run_todo() waits for the last references to an
unregistered device, netdev_wait_allrefs_any() resends NETDEV_UNREGISTER
about once per second. By then the device is no longer on its netns
device list. Since commit 8a4fc54b07d7 ("net: get rid of
rtnl_lock_unregistering()"), netns teardown does not wait for such
devices. So cleanup_net() can run all pernet exit handlers for the
device's netns while the device still points to it.

netdev_genl_netdevice_event() handles every NETDEV_UNREGISTER the same
way. On a rebroadcast it calls genl_has_listeners() on dev_net(dev). If
genl_pernet_exit() already ran for that netns, net->genl_sock is NULL:

  CPU0                                 CPU1
  unregister_netdevice_many_notify()
    unlist_netdevice(dev)
    NETDEV_UNREGISTER -> DEL_NTF sent
    net_set_todo(dev)
  netdev_run_todo()
    netdev_wait_allrefs_any()
                                       cleanup_net(netA)
                                         default_device_exit_batch()
                                           (dev not on netA's list)
                                         genl_pernet_exit(netA)
                                           netA->genl_sock = NULL
      NETDEV_UNREGISTER rebroadcast
        netdev_genl_dev_notify(dev)
          genl_has_listeners(netA)
            netlink_has_listeners(NULL)

If a later cleanup_net() run has already freed the struct net,
dev_net(dev) is a dangling pointer.

The splat below came from a 6.6.119-based kernel. The test kept creating
and destroying SR-IOV VFs on two PFs. It also kept binding and unbinding
the PCI devices and moving the VFs through short-lived namespaces. The
VF driver is out of tree:

  BUG: kernel NULL pointer dereference, address: 0000000000000388
  RIP: 0010:netlink_has_listeners+0x5/0x60
  Call Trace:
   netdev_genl_dev_notify+0x42/0x120
   netdev_genl_netdevice_event+0x2e/0x60
   notifier_call_chain+0x56/0xc0
   netdev_wait_allrefs_any+0x1ac/0x240
   netdev_run_todo+0x101/0x330
   jmnd_vnic_remove+0xb1/0x220 [xline_nic]
   auxiliary_bus_remove+0x18/0x30
   device_release_driver_internal+0x19b/0x200
   ...
   unbind_store+0x9d/0xb0

Current mainline has the same code paths. This workload has not been run
on mainline.

The first NETDEV_UNREGISTER is sent while reg_state is
NETREG_UNREGISTERING, under RTNL and before netns teardown can pass
default_device_exit_batch(), so the device-delete notification has
already gone out by then. netdev_run_todo() sets NETREG_UNREGISTERED
before it starts waiting, which is how the rebroadcast can be told
apart. Skip the rebroadcast, as caif_usb and mlxsw already do. This
also stops a duplicate NETDEV_CMD_DEV_DEL_NTF going to user space every
second while the device is stuck.

Fixes: d3d854fd6a1d ("netdev-genl: create a simple family for netdev stuff")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Xiaoguang Wang <lege.wang@jaguarmicro.com>
---
I'm not a networking developer. I hit this crash in our SR-IOV stress
testing and am mainly reporting the bug. The change below is the
simplest fix I could find; if it should be fixed elsewhere, e.g. in
netdev_wait_allrefs_any(), I'd appreciate guidance or am happy for
someone more familiar with this code to take it over.

 net/core/netdev-genl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index c3aa367214ac..0bd6bbbad720 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -1506,6 +1506,13 @@ static int netdev_genl_netdevice_event(struct notifier_block *nb,
 		netdev_unlock_full_to_ops(netdev);
 		break;
 	case NETDEV_UNREGISTER:
+		/* netdev_wait_allrefs_any() rebroadcasts NETDEV_UNREGISTER
+		 * while waiting for references. The device is already off the
+		 * netns device list, so its netns may be gone, and DEL_NTF
+		 * was sent on the first event.
+		 */
+		if (netdev->reg_state == NETREG_UNREGISTERED)
+			break;
 		netdev_lock(netdev);
 		netdev_genl_dev_notify(netdev, NETDEV_CMD_DEV_DEL_NTF);
 		netdev_unlock(netdev);

base-commit: 7076f684d5132e6a6145e1da2fa74771ecd052e5
-- 
2.43.0

Re: [RFC net] netdev-genl: skip DEL_NTF on NETDEV_UNREGISTER rebroadcast

[Stanislav Fomichev]

On 10/26, Xiaoguang Wang wrote:
> While netdev_run_todo() waits for the last references to an
> unregistered device, netdev_wait_allrefs_any() resends NETDEV_UNREGISTER
> about once per second. By then the device is no longer on its netns
> device list. Since commit 8a4fc54b07d7 ("net: get rid of
> rtnl_lock_unregistering()"), netns teardown does not wait for such
> devices. So cleanup_net() can run all pernet exit handlers for the
> device's netns while the device still points to it.
> 
> netdev_genl_netdevice_event() handles every NETDEV_UNREGISTER the same
> way. On a rebroadcast it calls genl_has_listeners() on dev_net(dev). If
> genl_pernet_exit() already ran for that netns, net->genl_sock is NULL:
> 
>   CPU0                                 CPU1
>   unregister_netdevice_many_notify()
>     unlist_netdevice(dev)
>     NETDEV_UNREGISTER -> DEL_NTF sent
>     net_set_todo(dev)
>   netdev_run_todo()
>     netdev_wait_allrefs_any()
>                                        cleanup_net(netA)
>                                          default_device_exit_batch()
>                                            (dev not on netA's list)
>                                          genl_pernet_exit(netA)
>                                            netA->genl_sock = NULL
>       NETDEV_UNREGISTER rebroadcast
>         netdev_genl_dev_notify(dev)
>           genl_has_listeners(netA)
>             netlink_has_listeners(NULL)
> 
> If a later cleanup_net() run has already freed the struct net,
> dev_net(dev) is a dangling pointer.
> 
> The splat below came from a 6.6.119-based kernel. The test kept creating
> and destroying SR-IOV VFs on two PFs. It also kept binding and unbinding
> the PCI devices and moving the VFs through short-lived namespaces. The
> VF driver is out of tree:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000388
>   RIP: 0010:netlink_has_listeners+0x5/0x60
>   Call Trace:
>    netdev_genl_dev_notify+0x42/0x120
>    netdev_genl_netdevice_event+0x2e/0x60
>    notifier_call_chain+0x56/0xc0
>    netdev_wait_allrefs_any+0x1ac/0x240
>    netdev_run_todo+0x101/0x330
>    jmnd_vnic_remove+0xb1/0x220 [xline_nic]
>    auxiliary_bus_remove+0x18/0x30
>    device_release_driver_internal+0x19b/0x200
>    ...
>    unbind_store+0x9d/0xb0
> 
> Current mainline has the same code paths. This workload has not been run
> on mainline.
> 
> The first NETDEV_UNREGISTER is sent while reg_state is
> NETREG_UNREGISTERING, under RTNL and before netns teardown can pass
> default_device_exit_batch(), so the device-delete notification has
> already gone out by then. netdev_run_todo() sets NETREG_UNREGISTERED
> before it starts waiting, which is how the rebroadcast can be told
> apart. Skip the rebroadcast, as caif_usb and mlxsw already do. This
> also stops a duplicate NETDEV_CMD_DEV_DEL_NTF going to user space every
> second while the device is stuck.
> 
> Fixes: d3d854fd6a1d ("netdev-genl: create a simple family for netdev stuff")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Xiaoguang Wang <lege.wang@jaguarmicro.com>
> ---
> I'm not a networking developer. I hit this crash in our SR-IOV stress
> testing and am mainly reporting the bug. The change below is the
> simplest fix I could find; if it should be fixed elsewhere, e.g. in
> netdev_wait_allrefs_any(), I'd appreciate guidance or am happy for
> someone more familiar with this code to take it over.

Note, same patch has been reposed here:

https://lore.kernel.org/netdev/20261026221306.3151025-1-namangulati@google.com/