Re: [PATCH] nbd: fix circular lock dependency

[Guilherme Giacomo Simoes]

Guilherme Giacomo Simoes <trintaeoitogc@xxxxxxxxx> wrote:
> > 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? 
> 
I back to read the nbd_genl_reconnect_sock_cb(), and now I understand what you say.
YES, nbd_get_socket() also runs for sockets that are never installed.  When
nbd_genl_reconnect_sock_cb() finds no dead connection it does:
```
	sockfd_put(sock);
	kfree(args);
	return 1; 
```

So the add line inside nbd_add_socket() that I sended previous don't make
sense.

What do you think now? We will call nbd_reclassify_socket() inside
nbd_get_socket() and call this only one time? Or is a better way call the
nbd_reclassify_socket() inside nbd_genl_reconnect_sock_cb() when we have sure
that we will use the socket?

thanks,
guilherme