Re: [PATCH] net: linkwatch: ignore events for unregistered netdevs

[Lukas Wunner <lukas@wunner.de>]

On Sat, Apr 23, 2022 at 06:07:23PM +0200, Lukas Wunner wrote:
> On Thu, Apr 21, 2022 at 10:02:43AM +0200, Paolo Abeni wrote:
> > On Sun, 2022-04-17 at 09:04 +0200, Lukas Wunner wrote:
> > > --- a/net/core/link_watch.c
> > > +++ b/net/core/link_watch.c
> > > @@ -107,7 +107,8 @@ static void linkwatch_add_event(struct net_device *dev)
> > >  	unsigned long flags;
> > >  
> > >  	spin_lock_irqsave(&lweventlist_lock, flags);
> > > -	if (list_empty(&dev->link_watch_list)) {
> > > +	if (list_empty(&dev->link_watch_list) &&
> > > +	    dev->reg_state < NETREG_UNREGISTERED) {
> > >  		list_add_tail(&dev->link_watch_list, &lweventlist);
> > >  		dev_hold_track(dev, &dev->linkwatch_dev_tracker, GFP_ATOMIC);
> > >  	
> > 
> > What about testing dev->reg_state in linkwatch_fire_event() before
> > setting the __LINK_STATE_LINKWATCH_PENDING bit, so that we don't leave
> > the device in an unexpected state?

About __LINK_STATE_LINKWATCH_PENDING being set even though the netdev
is not on link_watch_list:

After this patch (which removes one user of __LINK_STATE_LINKWATCH_PENDING)
the only purpose of the flag is a small speed-up of linkwatch_fire_event():
If the netdev is already on link_watch_list, the function skips acquiring
lweventlist_lock.

I don't think this is a hotpath, so the small speed-up is probably not worth
it and the flag could be removed completely in a follow-up patch.

There is a single other (somewhat oddball) user of the flag in
bond_should_notify_peers() in drivers/net/bonding/bond_main.c.
It would be possible to replace it with "!list_empty(&dev->link_watch_list)".
I don't think acquiring lweventlist_lock is necessary for that because
test_bit() is unordered (per Documentation/atomic_bitops.txt) and the
check is racy anyway.

Thanks,

Lukas