Commit 0b5c21bb authored by Johannes Berg's avatar Johannes Berg Committed by Jakub Kicinski
Browse files

net: ensure net_todo_list is processed quickly 

In [1], Will raised a potential issue that the cfg80211 code,
which does (from a locking perspective)

  rtnl_lock()
  wiphy_lock()
  rtnl_unlock()

might be suspectible to ABBA deadlocks, because rtnl_unlock()
calls netdev_run_todo(), which might end up calling rtnl_lock()
again, which could then deadlock (see the comment in the code
added here for the scenario).

Some back and forth and thinking ensued, but clearly this can't
happen if the net_todo_list is empty at the rtnl_unlock() here.
Clearly, the code here cannot actually put an entry on it, and
all other users of rtnl_unlock() will empty it since that will
always go through netdev_run_todo(), emptying the list.

So the only other way to get there would be to add to the list
and then unlock the RTNL without going through rtnl_unlock(),
which is only possible through __rtnl_unlock(). However, this
isn't exported and not used in many places, and none of them
seem to be able to unregister before using it.

Therefore, add a WARN_ON() in the code to ensure this invariant
won't be broken, so that the cfg80211 (or any similar) code
stays safe.

[1] https://lore.kernel.org/r/Yjzpo3TfZxtKPMAG@google.com



Signed-off-by: default avatarJohannes Berg <johannes.berg@intel.com>
Link: https://lore.kernel.org/r/20220404113847.0ee02e4a70da.Ic73d206e217db20fd22dcec14fe5442ca732804b@changeid


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent 6f2f36e5

include/linux/netdevice.h

+2 −1
Original line number Diff line number Diff line
@@ -3894,7 +3894,8 @@ void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev); 
extern int		netdev_budget;

extern unsigned int	netdev_budget_usecs;



/* Called by rtnetlink.c:rtnl_unlock() */

/* Used by rtnetlink.c:__rtnl_unlock()/rtnl_unlock() */

extern struct list_head net_todo_list;

void netdev_run_todo(void);



static inline void __dev_put(struct net_device *dev)

net/core/dev.c

+1 −1
Original line number Diff line number Diff line
@@ -9431,7 +9431,7 @@ static int dev_new_index(struct net *net) 
}



/* Delayed registration/unregisteration */

static LIST_HEAD(net_todo_list);

LIST_HEAD(net_todo_list);

DECLARE_WAIT_QUEUE_HEAD(netdev_unregistering_wq);



static void net_set_todo(struct net_device *dev)

net/core/rtnetlink.c

+33 −0
Original line number Diff line number Diff line
@@ -95,6 +95,39 @@ void __rtnl_unlock(void) 


	defer_kfree_skb_list = NULL;



	/* Ensure that we didn't actually add any TODO item when __rtnl_unlock()

	 * is used. In some places, e.g. in cfg80211, we have code that will do

	 * something like

	 *   rtnl_lock()

	 *   wiphy_lock()

	 *   ...

	 *   rtnl_unlock()

	 *

	 * and because netdev_run_todo() acquires the RTNL for items on the list

	 * we could cause a situation such as this:

	 * Thread 1			Thread 2

	 *				  rtnl_lock()

	 *				  unregister_netdevice()

	 *				  __rtnl_unlock()

	 * rtnl_lock()

	 * wiphy_lock()

	 * rtnl_unlock()

	 *   netdev_run_todo()

	 *     __rtnl_unlock()

	 *

	 *     // list not empty now

	 *     // because of thread 2

	 *				  rtnl_lock()

	 *     while (!list_empty(...))

	 *       rtnl_lock()

	 *				  wiphy_lock()

	 * **** DEADLOCK ****

	 *

	 * However, usage of __rtnl_unlock() is rare, and so we can ensure that

	 * it's not used in cases where something is added to do the list.

	 */

	WARN_ON(!list_empty(&net_todo_list));



	mutex_unlock(&rtnl_mutex);



	while (head) {