netdev - Re: [PATCH net-next v2 05/11] net: protect netdev->napi_list with netdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+o79p0tYH=ttRB7rQUp62fTrVXcxGQ3MN1vw9ZcoBg6w@mail.gmail.com>
Date: Fri, 17 Jan 2025 23:21:43 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: kuba@...nel.org, andrew+netdev@...n.ch, davem@...emloft.net, 
	horms@...nel.org, jdamato@...tly.com, netdev@...r.kernel.org, 
	pabeni@...hat.com
Subject: Re: [PATCH net-next v2 05/11] net: protect netdev->napi_list with netdev_lock()

On Wed, Jan 15, 2025 at 9:57 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Jakub Kicinski <kuba@...nel.org>
> Date: Tue, 14 Jan 2025 19:53:13 -0800
> > Hold netdev->lock when NAPIs are getting added or removed.
> > This will allow safe access to NAPI instances of a net_device
> > without rtnl_lock.
> >
> > Create a family of helpers which assume the lock is already taken.
> > Switch iavf to them, as it makes extensive use of netdev->lock,
> > already.
> >
> > Reviewed-by: Joe Damato <jdamato@...tly.com>
> > Reviewed-by: Eric Dumazet <edumazet@...gle.com>
> > Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.com>

Jakub, has anyone sent the following fix yet ?

CONFIG_DEBUG_MUTEXES=y should show a splat I think otherwise.

[1]
DEBUG_LOCKS_WARN_ON(lock->magic != lock)
WARNING: CPU: 0 PID: 5971 at kernel/locking/mutex.c:564
__mutex_lock_common kernel/locking/mutex.c:564 [inline]
WARNING: CPU: 0 PID: 5971 at kernel/locking/mutex.c:564
__mutex_lock+0xdac/0xee0 kernel/locking/mutex.c:735
Modules linked in:
CPU: 0 UID: 0 PID: 5971 Comm: syz-executor Not tainted
6.13.0-rc7-syzkaller-01131-g8d20dcda404d #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 12/27/2024
RIP: 0010:__mutex_lock_common kernel/locking/mutex.c:564 [inline]
RIP: 0010:__mutex_lock+0xdac/0xee0 kernel/locking/mutex.c:735
Code: 0f b6 04 38 84 c0 0f 85 1a 01 00 00 83 3d 6f 40 4c 04 00 75 19
90 48 c7 c7 60 84 0a 8c 48 c7 c6 00 85 0a 8c e8 f5 dc 91 f5 90 <0f> 0b
90 90 90 e9 c7 f3 ff ff 90 0f 0b 90 e9 29 f8 ff ff 90 0f 0b
RSP: 0018:ffffc90003317580 EFLAGS: 00010246
RAX: ee0f97edaf7b7d00 RBX: ffff8880299f8cb0 RCX: ffff8880323c9e00
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffc90003317710 R08: ffffffff81602ac2 R09: 1ffff110170c519a
R10: dffffc0000000000 R11: ffffed10170c519b R12: 0000000000000000
R13: 0000000000000000 R14: 1ffff92000662ec4 R15: dffffc0000000000
FS: 000055557a046500(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fd581d46ff8 CR3: 000000006f870000 CR4: 00000000003526f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
netdev_lock include/linux/netdevice.h:2691 [inline]
__netif_napi_del include/linux/netdevice.h:2829 [inline]
netif_napi_del include/linux/netdevice.h:2848 [inline]
free_netdev+0x2d9/0x610 net/core/dev.c:11621
netdev_run_todo+0xf21/0x10d0 net/core/dev.c:11189
nsim_destroy+0x3c3/0x620 drivers/net/netdevsim/netdev.c:1028
__nsim_dev_port_del+0x14b/0x1b0 drivers/net/netdevsim/dev.c:1428
nsim_dev_port_del_all drivers/net/netdevsim/dev.c:1440 [inline]
nsim_dev_reload_destroy+0x28a/0x490 drivers/net/netdevsim/dev.c:1661
nsim_drv_remove+0x58/0x160 drivers/net/netdevsim/dev.c:1676
device_remove drivers/base/dd.c:567 [inline]
__device_release_driver drivers/base/dd.c:1273 [inline]
device_release_driver_internal+0x4a9/0x7c0 drivers/base/dd.c:1296
bus_remove_device+0x34f/0x420 drivers/base/bus.c:576
device_del+0x57a/0x9b0 drivers/base/core.c:3854
device_unregister+0x20/0xc0 drivers/base/core.c:3895
nsim_bus_dev_del drivers/net/netdevsim/bus.c:462 [inline]
del_device_store+0x363/0x480 drivers/net/netdevsim/bus.c:226
kernfs_fop_write_iter+0x3a0/0x500 fs/kernfs/file.c:334
new_sync_write fs/read_write.c:586 [inline]
vfs_write+0xaeb/0xd30 fs/read_write.c:679
ksys_write+0x18f/0x2b0 fs/read_write.c:731
do_syscall_x64 arch/x86/entry/common.c:52 [inline]

diff --git a/net/core/dev.c b/net/core/dev.c
index fe5f5855593db34cb4bc31e6a637b59b9041bb73..fab4899b83f745a3c13c982775e287b1ff2f547d
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11593,8 +11593,6 @@ void free_netdev(struct net_device *dev)
                return;
        }

-       mutex_destroy(&dev->lock);
-
        kfree(dev->ethtool);
        netif_free_tx_queues(dev);
        netif_free_rx_queues(dev);
@@ -11621,6 +11619,8 @@ void free_netdev(struct net_device *dev)

        netdev_free_phy_link_topology(dev);

+       mutex_destroy(&dev->lock);
+
        /*  Compatibility with error handling in drivers */
        if (dev->reg_state == NETREG_UNINITIALIZED ||
            dev->reg_state == NETREG_DUMMY) {