Re: [PATCH net-next v10 08/14] net: hold netdev instance lock during sysfs operations

[Stanislav Fomichev <stfomichev@gmail.com>]

On 03/24, Cosmin Ratiu wrote:
> On Mon, 2025-03-24 at 09:06 -0700, Stanislav Fomichev wrote:
> > On 03/24, Cosmin Ratiu wrote:
> > > Call Trace:
> > > dump_stack_lvl+0x62/0x90
> > > print_deadlock_bug+0x274/0x3b0
> > > __lock_acquire+0x1229/0x2470
> > > lock_acquire+0xb7/0x2b0
> > > __mutex_lock+0xa6/0xd20
> > > dev_disable_lro+0x20/0x80
> > > inetdev_init+0x12f/0x1f0
> > > inetdev_event+0x48b/0x870
> > > notifier_call_chain+0x38/0xf0
> > > netif_change_net_namespace+0x72e/0x9f0
> > > do_setlink.isra.0+0xd5/0x1220
> > > rtnl_newlink+0x7ea/0xb50
> > > rtnetlink_rcv_msg+0x459/0x5e0
> > > netlink_rcv_skb+0x54/0x100
> > > netlink_unicast+0x193/0x270
> > > netlink_sendmsg+0x204/0x450
> > 
> > I think something like the patch below should fix it? inetdev_init is
> > called for blackhole (sw device, we don't care about ops lock) and
> > from
> > REGISTER/UNREGISTER notifiers. We hold the lock during REGISTER,
> > and will soon hold the lock during UNREGISTER:
> > https://lore.kernel.org/netdev/20250312223507.805719-9-kuba@kernel.org/
> > 
> > (might also need to EXPORT_SYM netif_disable_lro)
> > 
> > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> > index 754f60fb6e25..77e5705ac799 100644
> > --- a/net/ipv4/devinet.c
> > +++ b/net/ipv4/devinet.c
> > @@ -281,7 +281,7 @@ static struct in_device *inetdev_init(struct
> > net_device *dev)
> >  	if (!in_dev->arp_parms)
> >  		goto out_kfree;
> >  	if (IPV4_DEVCONF(in_dev->cnf, FORWARDING))
> > -		dev_disable_lro(dev);
> > +		netif_disable_lro(dev);
> >  	/* Reference in_dev->dev */
> >  	netdev_hold(dev, &in_dev->dev_tracker, GFP_KERNEL);
> >  	/* Account for reference dev->ip_ptr (below) */
> 
> Unfortunately, this seems to result, on another code path, in:
> WARNING: CPU: 10 PID: 1479 at ./include/net/netdev_lock.h:54
> __netdev_update_features+0x65f/0xca0
> __warn+0x81/0x180
> __netdev_update_features+0x65f/0xca0
> report_bug+0x156/0x180
> handle_bug+0x4f/0x90
> exc_invalid_op+0x13/0x60
> asm_exc_invalid_op+0x16/0x20
> __netdev_update_features+0x65f/0xca0
> netif_disable_lro+0x30/0x1d0
> inetdev_init+0x12f/0x1f0
> inetdev_event+0x48b/0x870
> notifier_call_chain+0x38/0xf0
> register_netdevice+0x741/0x8b0
> register_netdev+0x1f/0x40
> mlx5e_probe+0x4e3/0x8e0 [mlx5_core]
> auxiliary_bus_probe+0x3f/0x90
> really_probe+0xc3/0x3a0
> __driver_probe_device+0x80/0x150
> driver_probe_device+0x1f/0x90
> __device_attach_driver+0x7d/0x100
> bus_for_each_drv+0x80/0xd0
> __device_attach+0xb4/0x1c0
> bus_probe_device+0x91/0xa0
> device_add+0x657/0x870
> 
> I see register_netdevice briefly acquires the netdev lock in two
> separate blocks and has a __netdev_update_features call in one of the
> blocks, but the lock is not held for
> call_netdevice_notifiers(NETDEV_REGISTER, dev).

Ok, so we might need to also try to run NETDEV_REGISTER hooks
consistently under the instance lock. This might bring more surprises,
but I don't see any other easy option. Will test it out locally...

diff --git a/net/core/dev.c b/net/core/dev.c
index f29c1368c304..d672d521b92a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1815,7 +1815,9 @@ static int call_netdevice_register_notifiers(struct notifier_block *nb,
 {
 	int err;
 
+	netdev_lock_ops(dev);
 	err = call_netdevice_notifier(nb, NETDEV_REGISTER, dev);
+	netdev_unlock_ops(dev);
 	err = notifier_to_errno(err);
 	if (err)
 		return err;
@@ -11014,7 +11016,9 @@ int register_netdevice(struct net_device *dev)
 		memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
 
 	/* Notify protocols, that a new device appeared. */
+	netdev_lock_ops(dev);
 	ret = call_netdevice_notifiers(NETDEV_REGISTER, dev);
+	netdev_unlock_ops(dev);
 	ret = notifier_to_errno(ret);
 	if (ret) {
 		/* Expect explicit free_netdev() on failure */
@@ -12036,6 +12040,7 @@ int netif_change_net_namespace(struct net_device *dev, struct net *net,
 	int err, new_nsid;
 
 	ASSERT_RTNL();
+	netdev_ops_assert_locked(dev);
 
 	/* Don't allow namespace local devices to be moved. */
 	err = -EINVAL;