netdev - Re: [RFC PATCH net-next 8/9] net: delay device_del until run

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: <163290399584.3047.8100336131824633098@kwain>
Date:   Wed, 29 Sep 2021 10:26:35 +0200
From:   Antoine Tenart <atenart@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, pabeni@...hat.com, gregkh@...uxfoundation.org,
        ebiederm@...ssion.com, stephen@...workplumber.org,
        herbert@...dor.apana.org.au, juri.lelli@...hat.com,
        netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next 8/9] net: delay device_del until run_todo

Quoting Jakub Kicinski (2021-09-29 02:02:29)
> On Tue, 28 Sep 2021 14:54:59 +0200 Antoine Tenart wrote:
> > The sysfs removal is done in device_del, and moving it outside of the
> > rtnl lock does fix the initial deadlock. With that the trylock/restart
> > logic can be removed in a following-up patch.
> 
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index a1eab120bb50..d774fbec5d63 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -10593,6 +10593,8 @@ void netdev_run_todo(void)
> >                       continue;
> >               }
> >  
> > +             device_del(&dev->dev);
> > +
> >               dev->reg_state = NETREG_UNREGISTERED;
> >  
> >               netdev_wait_allrefs(dev);
> > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> > index 21c3fdeccf20..e754f00c117b 100644
> > --- a/net/core/net-sysfs.c
> > +++ b/net/core/net-sysfs.c
> > @@ -1955,8 +1955,6 @@ void netdev_unregister_kobject(struct net_device *ndev)
> >       remove_queue_kobjects(ndev);
> >  
> >       pm_runtime_set_memalloc_noio(dev, false);
> > -
> > -     device_del(dev);
> >  }
> >  
> >  /* Create sysfs entries for network device. */
> 
> Doesn't this mean there may be sysfs files which are accessible 
> for an unregistered netdevice?

It would mean having accessible sysfs files for a device in the
NETREG_UNREGISTERING state; NETREG_UNREGISTERED still comes after
device_del. It's a small difference but still important, I think.

You raise a good point. Yes, that would mean accessing attributes of net
devices being unregistered, meaning accessing or modifying unused or
obsolete parameters and data (it shouldn't be garbage data though).
Unlisting those sysfs files without removing them would be better here,
to not expose files when the device is being unregistered while still
allowing pending operations to complete. I don't know if that is doable
in sysfs.

(While I did ran stress tests reading/writing attributes while
unregistering devices, I think I missed an issue with the
netdev_queue_default attributes; which hopefully can be fixed — if the
whole idea is deemed acceptable).

> Isn't the point of having device_del() under rtnl_lock() to make sure
> we sysfs handlers can't run on dead devices?

Hard to say what was the initial point, there is a lot of history here
:) I'm not sure it was done because of a particular reason; IMHO it just
made sense to make this simple without having a good reason not to do
so. And it helped with the naming collision detection.

Thanks!
Antoine