Re: [PATCH] nbd: fix circular lock dependency

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

On 11/10/26 1:57 PM, Guilherme Giacomo Simoes wrote:
 > Subject: [PATCH] nbd: fix circular lock dependency

There is no real circular lock dependency here. This patch addresses a
lockdep false positive caused by the missing socket reclassification in
the reconnect path. Please change the subject into something like
"nbd: Also reclassify sockets in the reconnect path".

 > instead of the NBD-specific class leading to a possible lock dependency.

Please change "possible lock dependency" into "lockdep false positive".

 > Reported-by: syzbot+d7d72f3d467710442af0@syzkaller.appspotmail.com
 > Link: https://syzkaller.appspot.com/bug?extid=d7d72f3d467710442af0

Please use Closes: instead of Link: for the syzbot report. Has this
patch been tested against the syzbot reproducer, e.g. with "#syz test"?
If so, please mention this.

 > -static struct socket *nbd_get_socket(struct nbd_device *nbd, 
unsigned long fd,
 > -				     int *err)
 > -{
[ ... ]
 > +static struct socket *nbd_get_socket(struct nbd_device *nbd, 
unsigned long fd,
 > +				     int *err)
 > +{

Moving nbd_get_socket() makes this patch much larger than necessary,
which makes reviewing and backporting it harder. Adding a single
nbd_reclassify_socket(sock) call in nbd_genl_reconnect_sock_cb() right
after the nbd_get_socket() call is sufficient. If the reclassification
should happen in nbd_get_socket(), please move nbd_reclassify_socket()
up instead of moving nbd_get_socket() down.

Additionally, reclassifying the socket in nbd_get_socket() happens
before it is known whether NBD will use the socket. If
nbd_genl_reconnect_sock_cb() does not find a dead connection, or if
nbd_add_socket() fails, the socket is returned to user space with the
NBD lock class. While this only affects lockdep and while the
nbd_add_socket() error paths already behave this way, it is another
reason to only reclassify a socket once it is really being used.

 > +	/*
 > +	 * NBD sockets are used from block/swap/writeback context, which can
 > +	 * already hold locks that fs_reclaim depends on.  Give them their own
 > +	 * lockdep class so that a page fault on an unrelated userspace socket
 > +	 * (sk_lock -> mmap_lock) is not chained to the NBD socket lock.
 > +	 */

The description of commit d532cddb6c60 says that the problem is
fs_reclaim being triggered while sk_lock is held, not a page fault
(sk_lock -> mmap_lock). Please make this comment match the actual
lockdep report or leave it out. The description of commit d532cddb6c60
already explains why NBD sockets are reclassified.

 > +	if (sock)
 > +		nbd_reclassify_socket(sock);

"sock" can't be NULL here because all code paths where it is NULL have
already returned. Please remove the "if (sock)" test.

Thanks,

Bart.