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

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

[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>
---
 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;
 	}
 }
-- 
2.53.0

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

[Christian Borntraeger]

Am 13.07.26 um 20:14 schrieb Christian Borntraeger:

I just realized I had my kernel.org address in from.
question is, is this the right fix and shall I do a proper patch wit correct from and signoff.

[..]

> 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;
>   	}
>   }

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

[Christian Borntraeger]

> 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;
>   	}
>   }


FWIW, as sashiko pointed out, the mutex_lock check is racy;
it narrows the window but does not close it. In mirrors the existing
sk_lock path, which has the same property.

AFAIK, the consequences are confined to lockdep bookkeeping.
In practice the window is one-shot and unreachable for a functioning
client: the reclassification runs once at socket hand-over, and a
thread concurrently doing recvmsg() on the socket it just handed to
nbd would destroy the NBD protocol framing anyway.

So the patch is basically best effort to avoid lockdep being turned
off - which is my concern since this happens early during our CI runs.

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

[Christian Borntraeger]

Am 14.07.26 um 18:19 schrieb Christian Borntraeger:
> Am 13.07.26 um 20:14 schrieb Christian Borntraeger:
> 
> I just realized I had my kernel.org address in from.
> question is, is this the right fix and shall I do a proper patch wit correct from and signoff.
> 
> [..]
> 
>> 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;
>>       }
>>   }


And when you are at it, maybe also do the reclassification for NBD_CMD_RECONFIGURE

@@ -1371,6 +1371,7 @@ static int nbd_reconnect_socket(struct nbd_device *nbd, unsigned long arg)
         sock = nbd_get_socket(nbd, arg, &err);
         if (!sock)
                 return err;
+       nbd_reclassify_socket(sock);
  
         args = kzalloc_obj(*args);
         if (!args) {