netdev - Re: [PATCH net 2/2] net: avoid false positive warnings in __net_mp_close

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250402162428.4afc90cb@kernel.org>
Date: Wed, 2 Apr 2025 16:24:28 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>, sdf@...ichev.me
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
 ap420073@...il.com, asml.silence@...il.com, dw@...idwei.uk
Subject: Re: [PATCH net 2/2] net: avoid false positive warnings in
 __net_mp_close_rxq()

On Wed, 2 Apr 2025 11:52:50 -0700 Mina Almasry wrote:
> >         netdev_lock(dev);
> > -       __net_mp_close_rxq(dev, ifq_idx, old_p);
> > +       /* Callers holding a netdev ref may get here after we already
> > +        * went thru shutdown via dev_memory_provider_uninstall().
> > +        */
> > +       if (dev->reg_state <= NETREG_REGISTERED)
> > +               __net_mp_close_rxq(dev, ifq_idx, old_p);  
> 
> Not obvious to me why this check was moved. Do you expect to call
> __net_mp_close_rxq on an unregistered netdev and expect it to succeed
> in io_uring binding or something?

Yes, iouring state is under spin lock it can't call in here atomically.
device unregister may race with iouring shutdown.

Now that I look at it I think we need

diff --git a/net/core/dev.c b/net/core/dev.c
index be17e0660144..0a70080a1209 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11947,6 +11947,7 @@ void unregister_netdevice_many_notify(struct list_head *head,
                unlist_netdevice(dev);
                netdev_lock(dev);
                WRITE_ONCE(dev->reg_state, NETREG_UNREGISTERING);
+               dev_memory_provider_uninstall(dev);
                netdev_unlock(dev);
        }
        flush_all_backlogs();
@@ -11961,7 +11962,6 @@ void unregister_netdevice_many_notify(struct list_head *head,
                dev_tcx_uninstall(dev);
                netdev_lock_ops(dev);
                dev_xdp_uninstall(dev);
-               dev_memory_provider_uninstall(dev);
                netdev_unlock_ops(dev);
                bpf_dev_bound_netdev_unregister(dev);

since 1d22d3060b9b ("net: drop rtnl_lock for queue_mgmt operations")
we drop the lock after setting UNREGISTERING so we may call .uninstall
after iouring torn down its side.

Right, Stan?