Re: [PATCH] nbd: fix circular lock dependency

[Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>]

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