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

[Paolo Abeni <pabeni@redhat.com>]

On Sun, 2022-04-17 at 09:04 +0200, Lukas Wunner wrote:
> Jann Horn reports a use-after-free on disconnect of a USB Ethernet
> (ax88179_178a.c).  Oleksij Rempel has witnessed the same issue with a
> different driver (ax88172a.c).
> 
> Jann's report (linked below) explains the root cause in great detail,
> but the gist is that USB Ethernet drivers call linkwatch_fire_event()
> between unregister_netdev() and free_netdev().  The asynchronous work
> linkwatch_event() may thus access the netdev after it's been freed.
> 
> USB Ethernet may not even be the only culprit.  To address the problem
> in the most general way, ignore link events once a netdev's state has
> been set to NETREG_UNREGISTERED.
> 
> That happens in netdev_run_todo() immediately before the call to
> linkwatch_forget_dev().  Note that lweventlist_lock (and its implied
> memory barrier) guarantees that a linkwatch_add_event() running after
> linkwatch_forget_dev() will see the netdev's new state and bail out.
> An unregistered netdev is therefore never added to link_watch_list
> (but may have its __LINK_STATE_LINKWATCH_PENDING bit set, which should
> not matter).  That obviates the need to invoke linkwatch_run_queue() in
> netdev_wait_allrefs(), so drop it.
> 
> In a sense, the present commit is to *no longer* registered netdevs as
> commit b47300168e77 ("net: Do not fire linkwatch events until the device
> is registered.") is to *not yet* registered netdevs.
> 
> Reported-by: Jann Horn <jannh@google.com>
> Link: https://lore.kernel.org/netdev/CAG48ez0MHBbENX5gCdHAUXZ7h7s20LnepBF-pa5M=7Bi-jZrEA@mail.gmail.com/
> Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Link: https://lore.kernel.org/netdev/20220315113841.GA22337@pengutronix.de/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Oliver Neukum <oneukum@suse.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> ---
>  include/linux/netdevice.h |  2 --
>  net/core/dev.c            | 17 -----------------
>  net/core/link_watch.c     | 10 ++--------
>  3 files changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 59e27a2b7bf0..5d950b45b59d 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4805,8 +4805,6 @@ extern const struct kobj_ns_type_operations net_ns_type_operations;
>  
>  const char *netdev_drivername(const struct net_device *dev);
>  
> -void linkwatch_run_queue(void);
> -
>  static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
>  							  netdev_features_t f2)
>  {
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 8c6c08446556..0ee56965ff76 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10140,23 +10140,6 @@ static struct net_device *netdev_wait_allrefs_any(struct list_head *list)
>  			list_for_each_entry(dev, list, todo_list)
>  				call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
>  
> -			__rtnl_unlock();
> -			rcu_barrier();
> -			rtnl_lock();
> -
> -			list_for_each_entry(dev, list, todo_list)
> -				if (test_bit(__LINK_STATE_LINKWATCH_PENDING,
> -					     &dev->state)) {
> -					/* We must not have linkwatch events
> -					 * pending on unregister. If this
> -					 * happens, we simply run the queue
> -					 * unscheduled, resulting in a noop
> -					 * for this device.
> -					 */
> -					linkwatch_run_queue();
> -					break;
> -				}
> -
>  			__rtnl_unlock();
>  
>  			rebroadcast_time = jiffies;
> diff --git a/net/core/link_watch.c b/net/core/link_watch.c
> index 95098d1a49bd..9a0ea7cd68e4 100644
> --- 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?

Other than that, it looks good to me, but potentially quite risky. 

Looking at the original report it looks like the issue could be
resolved with a more usb-specific change: e.g. it looks like
usbnet_defer_kevent() is not acquiring a dev reference as it should.

Have you considered that path?

Thanks,

Paolo