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 Wed, 2025-03-05 at 08:37 -0800, Stanislav Fomichev wrote:
> > diff --git a/net/core/dev_api.c b/net/core/dev_api.c
> > index 059413d9ef9d..7bd667b34b80 100644
> > --- a/net/core/dev_api.c
> > +++ b/net/core/dev_api.c
> > +
> > +/**
> > + * dev_disable_lro() - disable Large Receive Offload on a device
> > + * @dev: device
> > + *
> > + * Disable Large Receive Offload (LRO) on a net device.  Must be
> > + * called under RTNL.  This is needed if received packets may be
> > + * forwarded to another interface.
> > + */
> > +void dev_disable_lro(struct net_device *dev)
> > +{
> > +	netdev_lock_ops(dev);
> > +	netif_disable_lro(dev);
> > +	netdev_unlock_ops(dev);
> > +}
> 
> It seems this part plus the following part from patch 6 of this series
> result in a recursive deadlock when inet forwarding is not enabled:
> 
> > @@ -3013,6 +3021,8 @@ static int do_setlink(const struct sk_buff
> > *skb, struct net_device *dev,
> >  	char ifname[IFNAMSIZ];
> >  	int err;
> >  
> > +	netdev_lock_ops(dev);
> > +
> >  	err = validate_linkmsg(dev, tb, extack);
> >  	if (err < 0)
> >  		goto errout;
> > 
> 
> 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
> 
> inetdev_init conditionally disables LRO if forwarding is on:
>         if (IPV4_DEVCONF(in_dev->cnf, FORWARDING)) 
>                 dev_disable_lro(dev);
> 
> What to do in this case (besides the silly workaround to disable
> forwarding)?

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) */