[PATCH] nbd: do not reinitialize a live socket lock when reclassifying

[PATCH] nbd: do not reinitialize a live socket lock when reclassifying

[Aibing Chen]

d532cddb6c60 ("nbd: Reclassify sockets to avoid lockdep circular
dependency") reclassifies the socket passed via NBD_SET_SOCK (or the
netlink equivalent) with sock_lock_init_class_and_name().  That helper
is meant for freshly created sockets: besides setting the lockdep
class it clears sk_lock.owned, reinitializes the wait queue and
reinitializes sk_lock.slock with spin_lock_init().

The NBD protocol has userspace keep the socket fd open, so the socket
can be in active use while it is being reclassified: an inet_shutdown()
or lock_sock() racing the reclassification can hold sk_lock.slock
across spin_lock_init(), leaving the live lock zeroed.  On
DEBUG_LOCK_ALLOC kernels this surfaces as "BUG: spinlock already
unlocked" (debug_spin_unlock()) from the shutdown/lock_sock path
running in parallel.

Add sock_lock_reclassify_and_name(), a variant that only re-arms the
lockdep class and name of the live sk_lock (plus the module owner
tracking) without touching the lock itself, sk_lock.owned or the wait
queue, and switch nbd_reclassify_socket() to it.  The
sock_allow_reclassification() guard is kept to narrow the window in
which the lock is in use by lock_sock().

Reported-by: syzbot+b29ed72382b0e218323a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b29ed72382b0e218323a
Fixes: d532cddb6c60 ("nbd: Reclassify sockets to avoid lockdep circular dependency")
Cc: stable@vger.kernel.org
Signed-off-by: Aibing Chen <chenaibing@cmss.chinamobile.com>
---
 drivers/block/nbd.c |  6 +++---
 include/net/sock.h  | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ffce519bf..28274a48d 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1251,19 +1251,19 @@ static void nbd_reclassify_socket(struct socket *sock)
 
 	switch (sk->sk_family) {
 	case AF_INET:
-		sock_lock_init_class_and_name(sk, "slock-AF_INET-NBD",
+		sock_lock_reclassify_and_name(sk, "slock-AF_INET-NBD",
 					      &nbd_slock_key[0],
 					      "sk_lock-AF_INET-NBD",
 					      &nbd_key[0]);
 		break;
 	case AF_INET6:
-		sock_lock_init_class_and_name(sk, "slock-AF_INET6-NBD",
+		sock_lock_reclassify_and_name(sk, "slock-AF_INET6-NBD",
 					      &nbd_slock_key[1],
 					      "sk_lock-AF_INET6-NBD",
 					      &nbd_key[1]);
 		break;
 	case AF_UNIX:
-		sock_lock_init_class_and_name(sk, "slock-AF_UNIX-NBD",
+		sock_lock_reclassify_and_name(sk, "slock-AF_UNIX-NBD",
 					      &nbd_slock_key[2],
 					      "sk_lock-AF_UNIX-NBD",
 					      &nbd_key[2]);
diff --git a/include/net/sock.h b/include/net/sock.h
index 3144efad8..2b305777c 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1704,6 +1704,22 @@ do {									\
 	lockdep_init_map(&(sk)->sk_lock.dep_map, (name), (key), 0);	\
 } while (0)
 
+/*
+ * Reclassify the lockdep class of a live socket lock.  Unlike
+ * sock_lock_init_class_and_name() this variant does not touch the
+ * lock itself, sk_lock.owned or the wait queue, so it is safe on a
+ * socket that userspace still holds.  The caller should check
+ * sock_allow_reclassification() first to narrow the window in which
+ * the lock is in use by lock_sock().
+ */
+#define sock_lock_reclassify_and_name(sk, sname, skey, name, key)	\
+do {									\
+	sk_owner_set(sk, THIS_MODULE);					\
+	lockdep_set_class_and_name(&(sk)->sk_lock.slock,		\
+				   (skey), (sname));			\
+	lockdep_init_map(&(sk)->sk_lock.dep_map, (name), (key), 0);	\
+} while (0)
+
 static inline bool lockdep_sock_is_held(const struct sock *sk)
 {
 	return lockdep_is_held(&sk->sk_lock) ||
-- 
2.50.1.windows.1

Re: [PATCH] nbd: do not reinitialize a live socket lock when reclassifying

[Bart Van Assche]

On 11/14/26 07:05, Aibing Chen wrote:
 > +#define sock_lock_reclassify_and_name(sk, sname, skey, name, key)	\
 > +do {									\
 > +	sk_owner_set(sk, THIS_MODULE);					\
 > +	lockdep_set_class_and_name(&(sk)->sk_lock.slock,		\
 > +				   (skey), (sname));			\
 > +	lockdep_init_map(&(sk)->sk_lock.dep_map, (name), (key), 0);	\
 > +} while (0)

Unlike the other callers of sock_lock_init_class_and_name(), which run
on freshly created in-kernel sockets before the socket is bound,
connected, or exposed to userspace, NBD operates on an already-connected
socket whose file descriptor is still held by userspace. Removing
spin_lock_init() is not sufficient to make reclassifying a live socket
safe:

1. Unsynchronized lockdep_init_map_type() on a live lock is racy.
    sock_allow_reclassification() is an unsynchronized check and
    nbd_reclassify_socket() holds no lock on sk and does not disable
    BH/IRQs. Both lockdep_set_class_and_name() and lockdep_init_map()
    call lockdep_init_map_type(), which clears lock->class_cache[],
    writes lock->name, and then writes lock->key without synchronization.

    If another CPU (via a syscall on the shared socket fd or a concurrent
    NBD_SET_SOCK on another /dev/nbdX device) or a softirq/timer
    (bh_lock_sock_nested()) concurrently acquires sk->sk_lock or
    sk->sk_lock.slock, look_up_lock_class() can observe a torn
    (lock->name, lock->key) pair and trigger:

      WARN_ONCE(class->name != lock->name &&
                lock->key != &__lockdep_no_validate__,
                "Looking for class \"%s\" with key %ps, but found a 
different class \"%s\" with the same key\n", ...)

    or permanently register the global af_family_* key with the NBD class
    name.

    Additionally, lock_sock_nested() calls
    mutex_acquire(&sk->sk_lock.dep_map, ...) before acquiring
    sk->sk_lock.slock or setting sk->sk_lock.owned = 1, and keeps
    dep_map acquired in lockdep while sleeping in __lock_sock().
    Reinitializing dep_map while another task holds it in lockdep also
    breaks match_held_lock() when hlock->references is non-zero.

2. sk_owner_set(sk, THIS_MODULE) leaks module references on repeated
    calls. When CONFIG_PROVE_LOCKING && CONFIG_MODULES is enabled,
    sk_owner_set() unconditionally calls __module_get(owner) and
    overwrites sk->sk_owner, while sk_prot_free() only calls
    sk_owner_put(sk) once when the socket is freed. Because userspace
    retains the socket fd, passing the same fd to NBD_SET_SOCK multiple
    times (e.g. in an NBD_SET_SOCK / NBD_CLEAR_SOCK loop as in the syzbot
    reproducer, or across multiple NBD devices) leaks a module reference
    on THIS_MODULE on every call after the first.

3. Because userspace retains the socket fd, reclassifying the socket to
    sk_lock-AF_INET-NBD does not prevent userspace from calling
    sendmsg()/sendto() on that fd and faulting in user pages
    (&mm->mmap_lock) while holding sk_lock-AF_INET-NBD. That recreates
    the exact sk_lock-AF_INET-NBD -> &mm->mmap_lock -> fs_reclaim ->
    sk_lock-AF_INET-NBD circular dependency that commit d532cddb6c60
    ("nbd: Reclassify sockets to avoid lockdep circular dependency")
    attempted to avoid, as already reported by syzbot:
  
https://lore.kernel.org/lkml/6aec646f.aca85a03.3b899.0000.GAE@google.com/

A possible solution is as follows:
* Revert both commit d532cddb6c60 ("nbd: Reclassify sockets to avoid
   lockdep circular dependency") and 9280e6edf656 ("nbd: don't warn when
   reclassifying a busy socket lock") because live sockets can be
   reclassified safely.
* Defer socket TX and shutdown from nbd_queue_rq() to a workqueue.
   Make nbd_queue_rq() enqueue the request onto a TX queue and schedule a
   work item on an unbound workqueue allocated without WQ_MEM_RECLAIM,
   similar to lo->workqueue in loop.c or nvme_tcp_wq in tcp.c.

Thanks,

Bart.