Re: [PATCH net] net: Lock lower level devices when updating features

[Cosmin Ratiu <cratiu@nvidia.com>]

On Wed, 2025-05-07 at 15:13 +0000, Cosmin Ratiu wrote:
> > In any case, please hold off with picking this patch up, it seems
> > there's a possibility of a real deadlock. Here's the scenario:
> > 
> > ============================================
> > WARNING: possible recursive locking detected
> > --------------------------------------------
> > ethtool/44150 is trying to acquire lock:
> > ffff8881364e8c80 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
> > __netdev_update_features+0x31e/0xe20
> > 
> > but task is already holding lock:
> > ffff8881364e8c80 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
> > ethnl_set_features+0xbc/0x4b0
> > and the lock comparison function returns 0:
> > 
> > other info that might help us debug this:
> >  Possible unsafe locking scenario:
> > 
> >        CPU0
> >        ----
> >   lock(&dev_instance_lock_key#7);
> >   lock(&dev_instance_lock_key#7);
> > 
> >  *** DEADLOCK ***
> > 
> >  May be due to missing lock nesting notation
> > 
> > 3 locks held by ethtool/44150:
> >  #0: ffffffff830e5a50 (cb_lock){++++}-{4:4}, at: genl_rcv+0x15/0x40
> >  #1: ffffffff830cf708 (rtnl_mutex){+.+.}-{4:4}, at:
> > ethnl_set_features+0x88/0x4b0
> >  #2: ffff8881364e8c80 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
> > ethnl_set_features+0xbc/0x4b0
> > 
> > stack backtrace:
> > Call Trace:
> >  <TASK>
> >  dump_stack_lvl+0x69/0xa0
> >  print_deadlock_bug.cold+0xbd/0xca
> >  __lock_acquire+0x163c/0x2f00
> >  lock_acquire+0xd3/0x2e0
> >  __mutex_lock+0x98/0xf10
> >  __netdev_update_features+0x31e/0xe20
> >  netdev_update_features+0x1f/0x60
> >  vlan_device_event+0x57d/0x930 [8021q]
> >  notifier_call_chain+0x3d/0x100
> >  netdev_features_change+0x32/0x50
> >  ethnl_set_features+0x17e/0x4b0
> >  genl_family_rcv_msg_doit+0xe0/0x130
> >  genl_rcv_msg+0x188/0x290
> > [...]
> > 
> > I'm not sure how to solve this yet...
> > Cosmin.
> 
> If it's not clear, the problem is that:
> 1. the lower device is already ops locked
> 2. netdev_feature_change gets called.
> 3. __netdev_update_features gets called for the vlan (upper) dev.
> 4. It tries to acquire the same lock instance as 1 (this patch).
> 5. Deadlock.
> 
> One solution I can think of would be to run device notifiers for
> changing features outside the lock, it doesn't seem like the netdev
> lock has anything to do with what other devices might do with this
> information.
> 
> This can be triggered from many scenarios, I have another similar
> stack
> involving bonding.
> 
> What do you think?

All I could think of was to drop the lock during the
netdev_features_changed notifier calls, like in the following hunk.
I'm running this through regressions, let's see if it's a good idea or
not.

diff --git a/net/core/dev.c b/net/core/dev.c
index 1be7cb73a602..817fd5bc21b1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1514,7 +1514,12 @@ int dev_get_alias(const struct net_device *dev,
char *name, size_t len)
  */
 void netdev_features_change(struct net_device *dev)
 {
+	/* Drop the lock to avoid potential deadlocks from e.g. upper
dev
+	 * notifiers altering features of 'dev' and acquiring dev lock
again.
+	 */
+	netdev_unlock_ops(dev);
 	call_netdevice_notifiers(NETDEV_FEAT_CHANGE, dev);
+	netdev_lock_ops(dev);
 }
 EXPORT_SYMBOL(netdev_features_change);