| Message ID | 20250327135659.2057487-2-sdf@fomichev.me (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hold instance lock during NETDEV_UP/REGISTER/UNREGISTER | expand |
On Thu, 27 Mar 2025 06:56:49 -0700 Stanislav Fomichev wrote: > Cosmin reports the following deadlock: > 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 > > Switch to netif_disable_lro which assumes the caller holds the instance > lock. inetdev_init is called for blackhole device (which sw device and > doesn't grab instance lock) and from REGISTER/UNREGISTER notifiers. > We already hold the instance lock for REGISTER notifier during > netns change and we'll soon hold the lock during other paths. Reviewed-by: Jakub Kicinski <kuba@kernel.org>
On Thu, 27 Mar 2025 06:56:49 -0700 Stanislav Fomichev wrote:
> +EXPORT_SYMBOL(netif_disable_lro);
Actually EXPORT_IPV6_MOD() would do here, no?
We only need this export for V6?
On 03/27, Jakub Kicinski wrote: > On Thu, 27 Mar 2025 06:56:49 -0700 Stanislav Fomichev wrote: > > +EXPORT_SYMBOL(netif_disable_lro); > > Actually EXPORT_IPV6_MOD() would do here, no? > We only need this export for V6? This patch is touching v4 net/ipv4/devinet.c, so both :-(
On Thu, 27 Mar 2025 14:09:07 -0700 Stanislav Fomichev wrote: > On 03/27, Jakub Kicinski wrote: > > On Thu, 27 Mar 2025 06:56:49 -0700 Stanislav Fomichev wrote: > > > +EXPORT_SYMBOL(netif_disable_lro); > > > > Actually EXPORT_IPV6_MOD() would do here, no? > > We only need this export for V6? > > This patch is touching v4 net/ipv4/devinet.c, so both :-( IPv4 can't be a module tho, we're talking about an export..
On 03/27, Jakub Kicinski wrote: > On Thu, 27 Mar 2025 14:09:07 -0700 Stanislav Fomichev wrote: > > On 03/27, Jakub Kicinski wrote: > > > On Thu, 27 Mar 2025 06:56:49 -0700 Stanislav Fomichev wrote: > > > > +EXPORT_SYMBOL(netif_disable_lro); > > > > > > Actually EXPORT_IPV6_MOD() would do here, no? > > > We only need this export for V6? > > > > This patch is touching v4 net/ipv4/devinet.c, so both :-( > > IPv4 can't be a module tho, we're talking about an export.. Ah, that's true!
diff --git a/net/core/dev.c b/net/core/dev.c index be17e0660144..80523f75ee6b 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1771,6 +1771,7 @@ void netif_disable_lro(struct net_device *dev) netdev_unlock_ops(lower_dev); } } +EXPORT_SYMBOL(netif_disable_lro); /** * dev_disable_gro_hw - disable HW Generic Receive Offload on a device 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) */
Cosmin reports the following deadlock: 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 Switch to netif_disable_lro which assumes the caller holds the instance lock. inetdev_init is called for blackhole device (which sw device and doesn't grab instance lock) and from REGISTER/UNREGISTER notifiers. We already hold the instance lock for REGISTER notifier during netns change and we'll soon hold the lock during other paths. Reported-by: Cosmin Ratiu <cratiu@nvidia.com> Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs operations") Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> --- net/core/dev.c | 1 + net/ipv4/devinet.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)