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

[Antoine Tenart <atenart@kernel.org>]

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