[net-next,v2,1/1] net: linkwatch: ignore events for unregistered netdevs

Commit Message

Lukas Wunner June 12, 2022, 9:08 a.m. UTC

Upon unregistering, a netdev is deleted from the linkwatch event list by
linkwatch_forget_dev(), but nothing prevents a driver from re-adding it.

Such belated events cause a use-after-free if the linkwatch queue
happens to run after free_netdev().  Jann Horn and Oleksij Rempel
independently report such use-after-frees with different USB Ethernet
adapters.

netdev_wait_allrefs_any() attempts to wait for belated events, but
cannot catch any after it has returned.  Additionally it can get stuck
in an infinite loop if a driver keeps signaling link events.

Avoid both problems by ignoring events in linkwatch_add_event() once a
netdev's state has been set to NETREG_UNREGISTERED.

This state change happens in netdev_run_todo() right before
linkwatch_forget_dev().  linkwatch_add_event() is serialized with
linkwatch_forget_dev() through lweventlist_lock.  By checking the netdev
state under that lock, linkwatch_add_event() is guaranteed to never add
a netdev to the linkwatch queue after linkwatch_forget_dev().

It thus becomes unnecessary to wait for belated events in
netdev_wait_allrefs_any(), allowing us to simplify and speed up
unregistration.

The rationale of ignoring belated events is that nobody cares about
operstate changes of a netdev once it's unregistered.

A belated event will cause a netdev's __LINK_STATE_LINKWATCH_PENDING bit
to remain set perpetually.  That will speed up processing of further
belated events, as they are immediately ignored through the
test_and_set_bit() call in linkwatch_fire_event().

Note that we already ignore linkwatch events of *not yet* registered
netdevs since commit b47300168e77 ("net: Do not fire linkwatch events
until the device is registered.").  The present commit does the same for
*no longer* registered netdevs.

As an alternative to the present commit, I've considered amending USB
Ethernet drivers to avoid signaling belated events:  That is achieved by
holding a reference on the netdev while calling linkwatch_fire_event().
A concurrent netdev_wait_allrefs_any() is thus forced to keep spinning.
To avoid calling linkwatch_fire_event() after netdev_wait_allrefs_any()
has returned, the netdev's state must not be NETREG_UNREGISTERED.
Here's the resulting patch:
https://lore.kernel.org/netdev/[e-mail]/

I consider that alternative approach to be inferior:  It puts the onus
on drivers to call linkwatch_fire_event() only under specific conditions,
which is error-prone.  The present approach instead changes the core API
to be defensive and avoid use-after-frees even if the driver neglected
these checks.  While I've examined all USB drivers and provided fixes in
the above-linked patch, other network drivers may well remain vulnerable.
Another disadvantage of the alternative approach is that it inflates
code size, whereas the solution presented herein *reduces* LoC and
complexity.

Reported-by: Jann Horn <[e-mail]>
Link: https://lore.kernel.org/netdev/CAG48ez0MHBbENX5gCdHAUXZ7h7s20LnepBF-pa5M=[e-mail]/
Reported-by: Oleksij Rempel <[e-mail]>
Link: https://lore.kernel.org/netdev/[e-mail]/
Signed-off-by: Lukas Wunner <[e-mail]>
Cc: [e-mail]
Cc: Oliver Neukum <[e-mail]>
---
 net/core/dev.c        | 17 -----------------
 net/core/dev.h        |  1 -
 net/core/link_watch.c | 10 ++--------
 3 files changed, 2 insertions(+), 26 deletions(-)

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 8958c4227b67..bbfcd70b3e7c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10242,23 +10242,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/dev.h b/net/core/dev.h
index cbb8a925175a..dd0a47ddaac3 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -30,7 +30,6 @@  int __init dev_proc_init(void);
 
 void linkwatch_init_dev(struct net_device *dev);
 void linkwatch_forget_dev(struct net_device *dev);
-void linkwatch_run_queue(void);
 
 void dev_addr_flush(struct net_device *dev);
 int dev_addr_init(struct net_device *dev);
diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index aa6cb1f90966..20634a55e1ce 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -108,7 +108,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);
 		netdev_hold(dev, &dev->linkwatch_dev_tracker, GFP_ATOMIC);
 	}
@@ -251,13 +252,6 @@  void linkwatch_forget_dev(struct net_device *dev)
 }
 
 
-/* Must be called with the rtnl semaphore held */
-void linkwatch_run_queue(void)
-{
-	__linkwatch_run_queue(0);
-}
-
-
 static void linkwatch_event(struct work_struct *dummy)
 {
 	rtnl_lock();