[PATCH] nbd: fix circular lock dependency

[PATCH] nbd: fix circular lock dependency

[Guilherme Giacomo Simoes]

nbd_genl_reconnect_sock_cb() replaces a dead socket with a new one
obtained through nbd_get_socket(), but unlike nbd_add_socket() it never
runs the socket through nbd_reclassify_socket().  The replacement socket
therefore keeps the generic sk_lock-AF_INET/AF_INET6/AF_UNIX lock class
instead of the NBD-specific class leading to a possible lock dependency.

Move the reclassification into nbd_get_socket(), the single place where
sockets are acquired for NBD, so both the connect and reconnect paths
reclassify the socket.

nbd_add_socket() no longer needs to do it.

Reported-by: syzbot+d7d72f3d467710442af0@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=d7d72f3d467710442af0
Fixes: d532cddb6c60 ("nbd: Reclassify sockets to avoid lockdep circular dependency")
Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
 drivers/block/nbd.c | 66 +++++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ffce519bf008..bc74ddb1d9d1 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1210,34 +1210,6 @@ static blk_status_t nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return ret;
 }
 
-static struct socket *nbd_get_socket(struct nbd_device *nbd, unsigned long fd,
-				     int *err)
-{
-	struct socket *sock;
-
-	*err = 0;
-	sock = sockfd_lookup(fd, err);
-	if (!sock)
-		return NULL;
-
-	if (!sk_is_tcp(sock->sk) &&
-	    !sk_is_stream_unix(sock->sk)) {
-		dev_err(disk_to_dev(nbd->disk), "Unsupported socket: should be TCP or UNIX.\n");
-		*err = -EINVAL;
-		sockfd_put(sock);
-		return NULL;
-	}
-
-	if (sock->ops->shutdown == sock_no_shutdown) {
-		dev_err(disk_to_dev(nbd->disk), "Unsupported socket: shutdown callout must be supported.\n");
-		*err = -EINVAL;
-		sockfd_put(sock);
-		return NULL;
-	}
-
-	return sock;
-}
-
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 static struct lock_class_key nbd_key[3];
 static struct lock_class_key nbd_slock_key[3];
@@ -1274,6 +1246,43 @@ static void nbd_reclassify_socket(struct socket *sock)
 static inline void nbd_reclassify_socket(struct socket *sock) {}
 #endif
 
+static struct socket *nbd_get_socket(struct nbd_device *nbd, unsigned long fd,
+				     int *err)
+{
+	struct socket *sock;
+
+	*err = 0;
+	sock = sockfd_lookup(fd, err);
+	if (!sock)
+		return NULL;
+
+	if (!sk_is_tcp(sock->sk) &&
+	    !sk_is_stream_unix(sock->sk)) {
+		dev_err(disk_to_dev(nbd->disk), "Unsupported socket: should be TCP or UNIX.\n");
+		*err = -EINVAL;
+		sockfd_put(sock);
+		return NULL;
+	}
+
+	if (sock->ops->shutdown == sock_no_shutdown) {
+		dev_err(disk_to_dev(nbd->disk), "Unsupported socket: shutdown callout must be supported.\n");
+		*err = -EINVAL;
+		sockfd_put(sock);
+		return NULL;
+	}
+
+	/*
+	 * 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.
+	 */
+	if (sock)
+		nbd_reclassify_socket(sock);
+
+	return sock;
+}
+
 static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
 			  bool netlink)
 {
@@ -1296,7 +1305,6 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
 	sock = nbd_get_socket(nbd, arg, &err);
 	if (!sock)
 		return err;
-	nbd_reclassify_socket(sock);
 
 	if (!netlink && !nbd->task_setup &&
 	    !test_bit(NBD_RT_BOUND, &config->runtime_flags))
-- 
2.52.0

Re: [PATCH] nbd: fix circular lock dependency

[Bart Van Assche]

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.