Re: [PATCH net] netdev-genl: ignore NETDEV_UNREGISTER rebroadcasts

[Naman Gulati]

On Mon, Oct 26, 2026 at 3:28 PM Stanislav Fomichev <sdf.kernel@xxxxxxxxx> wrote:
>
> On 10/26, Naman Gulati wrote:
> > netdev_wait_allrefs_any() re-sends NETDEV_UNREGISTER every second while
> > it waits for the last references on a device to be dropped. By then
> > netdev_run_todo() has already marked the device NETREG_UNREGISTERED and
> > RTNL has been dropped at least once since the initial notification sent
> > by unregister_netdevice_many_notify().
> >
> > netdev_genl_netdevice_event() does not tell these rebroadcasts apart
> > from the initial notification and sends a NETDEV_CMD_DEV_DEL_NTF
> > multicast for each of them. At best this spams NETDEV_NLGRP_MGMT
> > listeners with duplicate notifications; at worst the pernet state of
> > the netns the device belongs to is already gone and the handler
> > dereferences a NULL pointer.
> >
> > default_device_exit_batch() only unregisters the devices still listed
> > in the exiting netns; it does not wait for in-flight unregistrations of
> > devices that were already unlisted (the barrier doing that was removed
> > by commit 8a4fc54b07d7 ("net: get rid of rtnl_lock_unregistering()")).
> > So when a device is unregistered by something other than the
> > cleanup_net() of its own netns while references on it are still held,
> > e.g. a veth whose peer in another netns gets deleted, and that netns
> > exits while the device is stuck in netdev_wait_allrefs_any(), the
> > remaining pernet exit handlers run and genl_pernet_exit() sets
> > net->genl_sock to NULL. The next rebroadcast then crashes in
> > genl_has_listeners():
> >
> >   BUG: KASAN: null-ptr-deref in netlink_has_listeners+0x3d/0x3f0 net/netlink/af_netlink.c:1388
> >   Read of size 8 at addr 0000000000000578 by task syz.0.0/5325
> >   Call Trace:
> >    <TASK>
> >    netlink_has_listeners+0x3d/0x3f0 net/netlink/af_netlink.c:1388
> >    genl_has_listeners include/net/genetlink.h:644 [inline]
> >    netdev_genl_dev_notify+0x161/0x450 net/core/netdev-genl.c:93
> >    netdev_genl_netdevice_event+0x98/0x3c0 net/core/netdev-genl.c:1491
> >    notifier_call_chain+0x1a5/0x3d0 kernel/notifier.c:85
> >    netdev_wait_allrefs_any net/core/dev.c:11690 [inline]
> >    netdev_run_todo+0x731/0x1060 net/core/dev.c:11808
> >    rtnl_unlock net/core/rtnetlink.c:157 [inline]
> >    rtnl_dellink+0x68f/0x810 net/core/rtnetlink.c:3707
> >
> > syzbot hit this by moving one end of a veth pair into a short-lived
> > netns while netdevsim's deferred FIB event processing still held a
> > reference on it, and then deleting the other end (repro [1]). While the
> > source of this bug is netdevsim deferring FIB processing for and holding
> > onto the device reference for O(seconds) which is being fixed
> > separately [2], any driver may introduce such a delay and trigger the
> > null-ptr-deref.
> >
> > Skip the rebroadcasts, which are the only NETDEV_UNREGISTER events
> > delivered with reg_state == NETREG_UNREGISTERED. The notifications sent
> > by unregister_netdevice_many_notify() and, for a device leaving a netns,
> > by __dev_change_net_namespace() are issued under RTNL in the same
> > critical section that unlists the device, before its netns can be torn
> > down, and keep being handled. This mirrors what ip6_route_dev_notify()
> > already does to cope with the rebroadcasts.
> >
> > This change should not break the UAPI spec as it defines dev-del-ntf as
> > a "Notification about device disappearing." which is a single event. The
> > rebroadcast in netdev_wait_allrefs_any() exists to re-kick in-kernel
> > subsystems holding references; netdev-genl happens to be on the notifier
> > chain and forwards it.
> >
> > [1] https://gist.github.com/NamanGulati/2d1b49b8757f9a88b0bd6e55018c02ae
> > [2] https://lore.kernel.org/r/20261023191517.607834-1-kuniyu@xxxxxxxxxx
> >
> > Fixes: d3d854fd6a1d ("netdev-genl: create a simple family for netdev stuff")
> > Reported-by: syzbot+1269fe6cc2016008124e@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://lore.kernel.org/netdev/6adf9f64.6200f071.51519.0ed1.GAE@xxxxxxxxxx/
> > Reviewed-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> > Signed-off-by: Naman Gulati <namangulati@xxxxxxxxxx>
> > ---
> > Note that the reproducer attached to the syzbot bug targets a different
> > bug (via devlink_port_notify() on netns teardown) that happens to share
> > the same crash title, not the one in the original reported crash report.
> > ---
> >  net/core/netdev-genl.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > index 33b9f4eb9565a..2032ab9a864c2 100644
> > --- a/net/core/netdev-genl.c
> > +++ b/net/core/netdev-genl.c
> > @@ -1487,6 +1487,12 @@ 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 the last references to be dropped; the
> > +              * netns may already be torn down by then. Notify only once.
> > +              */
>
> [..]
>
> > +             if (READ_ONCE(netdev->reg_state) == NETREG_UNREGISTERED)
> > +                     break;
>
> Same as
> https://lore.kernel.org/netdev/20261026063743.1797-1-lege.wang@xxxxxxxxxxxxxxx/
> ?

Yes, sorry, same as that patch - which I missed. The author sent that
as an RFC and mentioned they're happy for someone else to take it
over. I'm happy to iterate on this and credit them unless Xiaoguang
prefers to keep working on it.

>
> reg_state is protected by netdev, why not do this under netdev_lock?

No strong reason. I read it outside the lock because the rebroadcast
runs on the same thread that set NETREG_UNREGISTERED in
netdev_run_todo() so reading reg_state under the lock is not strictly
required, but we take the lock right below so can move it there if
prefered.


> Also seems like we are fixing a symptom, not the problem?

Since 8a4fc54b07d7 ("net: get rid of rtnl_lock_unregistering()"),
cleanup_net() doesn't wait for devices stuck in
netdev_wait_allrefs_any(), so the rebroadcast can reach notifiers
after the device's netns has already exited. However the rebroadcast
is only a retry to make ref holders let go, not a new event, yet
netdev-genl sends userspace another DEL_NTF for each one. I don't
think userspace should receive these redundant notifications, even if
the device still exists and we avoid the null-ptr-deref.
ip6_route_dev_notify() and mlxsw_sp_netdevice_unregister() already
skip handling the rebroadcasts the same way, by checking for
NETREG_UNREGISTERED.



pw-bot: changes-requested

>
> >               netdev_lock(netdev);
> >               netdev_genl_dev_notify(netdev, NETDEV_CMD_DEV_DEL_NTF);
> >               netdev_unlock(netdev);