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] [day] [month] [year] [list]

Message-ID: <20250402182719.26c390fe@kernel.org>
Date: Wed, 2 Apr 2025 18:27:19 -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 16:24:28 -0700 Jakub Kicinski wrote:
> 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?

Actually, if I don't split the check here things will just work. 
Let me do that in v2. Thanks for flagging!