[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.

Re: [PATCH] nbd: fix circular lock dependency

[Guilherme Giacomo Simoes]

Bart Van Assche <bvanassche@acm.org> wrote:
> 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.
Ok, I will send a V2

> Has this patch been tested against the syzbot reproducer, e.g. with "#syz
> test"? If so, please mention this.
No, my bad. I send a email to syzbot now to test this patch.
After tested, how can I mention this in my patch?

> 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
Yes, is sufficient but the if we need reclassify socket all times when I call
nbd_get_socket(), looks like a uneccessary repetitive code.
Of course, I don't know if future situations where nbd_get_socket() is called
will need nbd_reclassify_socket() but *now* is just that happen.

> If the reclassification should happen in nbd_get_socket(), please move
> nbd_reclassify_socket() up instead of moving nbd_get_socket() down.
I was have moved nbd_reclassify_socket up, just like you suggested, but git
don't understanded this very well. But I will find way to get it

> 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.
current the nbd_reclassify_socket() is called just below nbd_get_socket() call
inside nbd_add_socket():
```
 	sock = nbd_get_socket(nbd, arg, &err);
 	if (!sock)
 		return err;
	nbd_reclassify_socket(sock);
```

what have exactly same effect that call at end of nbd_get_socket(). I guess
that "reclassify a socket once it is really being used" must be done putting
nbd_reclassify_socket at end of nbd_add_socket() (before put_socket tag).
Maybe something like:
```
static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
			  bool netlink)
{
	struct nbd_config *config = nbd->config;
	struct socket *sock;
	struct nbd_sock **socks;
	struct nbd_sock *nsock;
	int err;

	/* Arg will be cast to int, check it to avoid overflow */
	if (arg > INT_MAX)
		return -EINVAL;

	if (nbd->pid) {
		dev_err(disk_to_dev(nbd->disk),
			"Cannot add socket to a running device\n");
		return -EBUSY;
	}

	sock = nbd_get_socket(nbd, arg, &err);
	if (!sock)
		return err;

	if (!netlink && !nbd->task_setup &&
	    !test_bit(NBD_RT_BOUND, &config->runtime_flags))
		nbd->task_setup = current;

	if (!netlink &&
	    (nbd->task_setup != current ||
	     test_bit(NBD_RT_BOUND, &config->runtime_flags))) {
		dev_err(disk_to_dev(nbd->disk),
			"Device being setup by another task");
		err = -EBUSY;
		goto put_socket;
	}

	nsock = kzalloc_obj(*nsock);
	if (!nsock) {
		err = -ENOMEM;
		goto put_socket;
	}

	socks = krealloc(config->socks, (config->num_connections + 1) *
			 sizeof(struct nbd_sock *), GFP_KERNEL);
	if (!socks) {
		kfree(nsock);
		err = -ENOMEM;
		goto put_socket;
	}

+   nbd_reclassify_socket(sock);

	config->socks = socks;

	nsock->fallback_index = -1;
	nsock->dead = false;
	mutex_init(&nsock->tx_lock);
	nsock->sock = sock;
	nsock->pending = NULL;
	nsock->sent = 0;
	nsock->cookie = 0;
	INIT_WORK(&nsock->work, nbd_pending_cmd_work);
	socks[config->num_connections++] = nsock;
	atomic_inc(&config->live_connections);

	return 0;

put_socket:
	sockfd_put(sock);
	return err;
}
```

I don't know if this is a really good thing once that until this be executed
the user space can got a generic lock (the same generic lock of sock here)
before nbd_reclassify_socket() call leading to the same problem.
Thoughts? 

>  > +	/*
>  > +	 * 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.
Ok, I will fix this two things

Thanks,
Guilherme