Re: [PATCH] nbd: reclassify u->iolock of AF_UNIX sockets

[Christian Borntraeger <borntraeger@linux.ibm.com>]

Am 13.07.26 um 20:14 schrieb Christian Borntraeger:
> From: Christian Borntraeger <borntraeger@kernel.org>
> 
> in our CI we do get the following when unloading the nbd module.
> 
> [  120.008698] block nbd0: NBD_DISCONNECT
> [  120.009078] block nbd0: Disconnected due to user request.
> [  120.009413] block nbd0: shutting down sockets
> 
> [  120.269755] ======================================================
> [  120.269757] WARNING: possible circular locking dependency detected
> [  120.269759] 7.2.0-rc2-00005-g3e3aa6da87d3 #117 Not tainted
> [  120.269761] ------------------------------------------------------
> [  120.269762] modprobe/8168 is trying to acquire lock:
> [  120.269764] 000001e67da57138 (set->srcu){.+.?}-{0:0}, at: __synchronize_srcu+0x36/0x210
> [  120.269775]
>                 but task is already holding lock:
> [  120.269776] 000001bf63462d90 (&q->elevator_lock){+.+.}-{3:3}, at: elevator_change+0xac/0x1d0
> [  120.269784] which lock already depends on the new lock.
> 
> [  120.269786] the existing dependency chain (in reverse order) is:
> [  120.269788]
>                 -> #5 (&q->elevator_lock){+.+.}-{3:3}:
> [  120.269792]        lock_acquire+0x150/0x3f0
> [  120.269795]        __mutex_lock+0xba/0xdc0
> [  120.269799]        mutex_lock_nested+0x32/0x40
> [  120.269802]        elevator_change+0xac/0x1d0
> [  120.269804]        elv_iosched_store+0x194/0x1e0
> [  120.269807]        kernfs_fop_write_iter+0x18c/0x270
> [  120.269810]        vfs_write+0x23c/0x380
> [  120.269813]        ksys_write+0x88/0x120
> [  120.269816]        __do_syscall+0x170/0x750
> [  120.269818]        system_call+0x72/0x90
> [  120.269821]
>                 -> #4 (&q->q_usage_counter(io)#18){++++}-{0:0}:
> [  120.269871]        lock_acquire+0x150/0x3f0
> [  120.269873]        blk_alloc_queue+0x2fe/0x340
> [  120.269876]        blk_mq_alloc_queue+0x6c/0xe0
> [  120.269879]        __blk_mq_alloc_disk+0x2a/0x80
> [  120.269881]        dasd_gendisk_alloc+0xd4/0x200
> [  120.269885]        dasd_state_known_to_basic+0x34/0x1f0
> [  120.269890]        dasd_change_state+0x28a/0x570
> [  120.269892]        dasd_set_target_state+0x76/0xf0
> [  120.269895]        dasd_generic_set_online+0xc8/0x2c0
> [  120.269897]        ccw_device_set_online+0xc6/0x550
> [  120.269901]        online_store+0xd2/0x270
> [  120.269904]        kernfs_fop_write_iter+0x18c/0x270
> [  120.269906]        vfs_write+0x23c/0x380
> [  120.269909]        ksys_write+0x88/0x120
> [  120.269912]        __do_syscall+0x170/0x750
> [  120.269914]        system_call+0x72/0x90
> [  120.269916]
>                 -> #3 (fs_reclaim){+.+.}-{0:0}:
> [  120.269920]        lock_acquire+0x150/0x3f0
> [  120.269922]        __fs_reclaim_acquire+0x44/0x50
> [  120.269925]        fs_reclaim_acquire+0xbe/0x100
> [  120.269927]        kmem_cache_alloc_noprof+0x6c/0x6d0
> [  120.269931]        alloc_empty_file+0x58/0x150
> [  120.269933]        dentry_open+0x3c/0xa0
> [  120.269935]        pidfs_alloc_file+0xba/0x130
> [  120.269940]        pidfd_prepare+0x8c/0xf0
> [  120.269945]        scm_recv_unix+0xb0/0x130
> [  120.269949]        __unix_dgram_recvmsg+0x2aa/0x430
> [  120.269954]        sock_recvmsg+0x70/0xd0
> [  120.269956]        ____sys_recvmsg+0x8a/0x190
> [  120.269958]        ___sys_recvmsg+0x88/0xd0
> [  120.269960]        __sys_recvmsg+0x76/0xd0
> [  120.269962]        __do_sys_socketcall+0x40a/0x460
> [  120.269964]        __do_syscall+0x170/0x750
> [  120.269967]        system_call+0x72/0x90
> [  120.269969]
>                 -> #2 (&u->iolock){+.+.}-{3:3}:
> [  120.269973]        lock_acquire+0x150/0x3f0
> [  120.269975]        __mutex_lock+0xba/0xdc0
> [  120.269978]        mutex_lock_nested+0x32/0x40
> [  120.269981]        unix_stream_read_generic+0x11e/0xa60
> [  120.269985]        unix_stream_recvmsg+0xae/0xc0
> [  120.269988]        sock_recvmsg+0x70/0xd0
> [  120.269990]        __sock_xmit+0xf0/0x1b0 [nbd]
> [  120.269997]        nbd_handle_reply+0x284/0x770 [nbd]
> [  120.270001]        recv_work+0x10c/0x290 [nbd]
> [  120.270004]        process_one_work+0x2ba/0x800
> [  120.270009]        worker_thread+0x21a/0x400
> [  120.270012]        kthread+0x164/0x190
> [  120.270016]        __ret_from_fork+0x4c/0x340
> [  120.270020]        ret_from_fork+0xa/0x30
> [  120.270022]
>                 -> #1 (&cmd->lock){+.+.}-{3:3}:
> [  120.270026]        lock_acquire+0x150/0x3f0
> [  120.270028]        __mutex_lock+0xba/0xdc0
> [  120.270031]        mutex_lock_nested+0x32/0x40
> [  120.270034]        nbd_queue_rq+0x38/0x70 [nbd]
> [  120.270037]        blk_mq_dispatch_rq_list+0x19c/0x540
> [  120.270040]        __blk_mq_do_dispatch_sched+0x3ce/0x3e0
> [  120.270043]        __blk_mq_sched_dispatch_requests+0x198/0x1e0
> [  120.270046]        blk_mq_sched_dispatch_requests+0x3c/0x90
> [  120.270049]        blk_mq_run_work_fn+0x9e/0x180
> [  120.270052]        process_one_work+0x2ba/0x800
> [  120.270055]        worker_thread+0x21a/0x400
> [  120.270058]        kthread+0x164/0x190
> [  120.270060]        __ret_from_fork+0x4c/0x340
> [  120.270063]        ret_from_fork+0xa/0x30
> [  120.270065]
>                 -> #0 (set->srcu){.+.?}-{0:0}:
> [  120.270069]        check_prev_add+0x160/0xf40
> [  120.270073]        __lock_acquire+0x12aa/0x15a0
> [  120.270075]        lock_sync+0xf4/0x190
> [  120.270077]        __synchronize_srcu+0x5c/0x210
> [  120.270080]        elevator_switch+0x190/0x340
> [  120.270082]        elevator_change+0x152/0x1d0
> [  120.270084]        elevator_set_none+0x44/0x90
> [  120.270087]        blk_unregister_queue+0xd4/0x130
> [  120.270090]        __del_gendisk+0x172/0x3c0
> [  120.270093]        del_gendisk+0x86/0xb0
> [  120.270096]        nbd_dev_remove+0x30/0x90 [nbd]
> [  120.270100]        nbd_cleanup+0x11a/0x3a8 [nbd]
> [  120.270104]        __do_sys_delete_module+0x18e/0x340
> [  120.270107]        __do_syscall+0x170/0x750
> [  120.270110]        system_call+0x72/0x90
> [  120.270112]
>                 other info that might help us debug this:
> 
> [  120.270114] Chain exists of:
>                   set->srcu --> &q->q_usage_counter(io)#18 --> &q->elevator_lock
> 
> [  120.270120]  Possible unsafe locking scenario:
> 
> [  120.270121]        CPU0                    CPU1
> [  120.270122]        ----                    ----
> [  120.270124]   lock(&q->elevator_lock);
> [  120.270126]                                lock(&q->q_usage_counter(io)#18);
> [  120.270129]                                lock(&q->elevator_lock);
> [  120.270132]   sync(set->srcu);
> [  120.270134]
>                  *** DEADLOCK ***
> 
> [  120.270136] 2 locks held by modprobe/8168:
> [  120.270138]  #0: 000001bf44ce11e0 (&set->update_nr_hwq_lock){++++}-{3:3}, at: del_gendisk+0x76/0xb0
> [  120.270145]  #1: 000001bf63462d90 (&q->elevator_lock){+.+.}-{3:3}, at: elevator_change+0xac/0x1d0
> [  120.270152]
>                 stack backtrace:
> [  120.270155] CPU: 48 UID: 0 PID: 8168 Comm: modprobe Not tainted 7.2.0-rc2-00005-g3e3aa6da87d3 #117 PREEMPT
> [  120.270157] Hardware name: IBM 9175 ME1 701 (LPAR)
> [  120.270158] Call Trace:
> [  120.270158]  [<000002c7d1164dde>] dump_stack_lvl+0xae/0x108
> [  120.270160]  [<000002c7d126e5b4>] print_circular_bug+0x1a4/0x230
> [  120.270163]  [<000002c7d126e7cc>] check_noncircular+0x18c/0x1b0
> [  120.270165]  [<000002c7d126fb80>] check_prev_add+0x160/0xf40
> [  120.270167]  [<000002c7d12736fa>] __lock_acquire+0x12aa/0x15a0
> [  120.270168]  [<000002c7d1275ce4>] lock_sync+0xf4/0x190
> [  120.270169]  [<000002c7d129c8fc>] __synchronize_srcu+0x5c/0x210
> [  120.270171]  [<000002c7d1bb4790>] elevator_switch+0x190/0x340
> [  120.270173]  [<000002c7d1bb4a92>] elevator_change+0x152/0x1d0
> [  120.270174]  [<000002c7d1bb53c4>] elevator_set_none+0x44/0x90
> [  120.270176]  [<000002c7d1bbee84>] blk_unregister_queue+0xd4/0x130
> [  120.270177]  [<000002c7d1bdff42>] __del_gendisk+0x172/0x3c0
> [  120.270179]  [<000002c7d1be0216>] del_gendisk+0x86/0xb0
> [  120.270182]  [<000002c751b2adc0>] nbd_dev_remove+0x30/0x90 [nbd]
> [  120.270185]  [<000002c751b30d72>] nbd_cleanup+0x11a/0x3a8 [nbd]
> [  120.270189]  [<000002c7d12c72ce>] __do_sys_delete_module+0x18e/0x340
> [  120.270191]  [<000002c7d22dc170>] __do_syscall+0x170/0x750
> [  120.270192]  [<000002c7d22f0db2>] system_call+0x72/0x90
> [  120.270194] INFO: lockdep is turned off.
> 
> this seems to be a false positive.
> 
> Commit d532cddb6c60 ("nbd: Reclassify sockets to avoid lockdep
> circular dependency") already reclassifies sk_lock and slock of
> sockets handed to nbd, including AF_UNIX ones, precisely to keep
> lockdep from reporting.
> That is not sufficient for AF_UNIX: the unix stream/dgram recvmsg and
> sendmsg paths do not serialize on sk_lock but on unix_sk(sk)->iolock,
> and that mutex keeps the generic "&u->iolock" class shared by every
> AF_UNIX socket in the system.
> 
> Teach lockdep about the lock instances to avoid this report.
> This gets rid of the lockdep warning.
> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>


Ping.

> ---
>   drivers/block/nbd.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 8f10762e90ef..a811e431b47a 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -32,6 +32,7 @@
>   #include <linux/err.h>
>   #include <linux/kernel.h>
>   #include <linux/slab.h>
> +#include <net/af_unix.h>
>   #include <net/sock.h>
>   #include <linux/net.h>
>   #include <linux/kthread.h>
> @@ -1241,6 +1242,7 @@ static struct socket *nbd_get_socket(struct nbd_device *nbd, unsigned long fd,
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   static struct lock_class_key nbd_key[3];
>   static struct lock_class_key nbd_slock_key[3];
> +static struct lock_class_key nbd_unix_iolock_key;
>   
>   static void nbd_reclassify_socket(struct socket *sock)
>   {
> @@ -1267,6 +1269,17 @@ static void nbd_reclassify_socket(struct socket *sock)
>   					      &nbd_slock_key[2],
>   					      "sk_lock-AF_UNIX-NBD",
>   					      &nbd_key[2]);
> +		/*
> +		 * The AF_UNIX stream recvmsg/sendmsg paths serialize on
> +		 * u->iolock, not sk_lock, so it must be reclassified as
> +		 * well.  A held mutex cannot be reclassified; skip it in
> +		 * that case, as sock_allow_reclassification() does for
> +		 * sk_lock.
> +		 */
> +		if (!mutex_is_locked(&unix_sk(sk)->iolock))
> +			lockdep_set_class_and_name(&unix_sk(sk)->iolock,
> +						   &nbd_unix_iolock_key,
> +						   "&u->iolock-NBD");
>   		break;
>   	}
>   }