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

[Bart Van Assche <bvanassche@acm.org>]

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.