[PATCH] nbd: verify socket is supported during setup

[PATCH] nbd: verify socket is supported during setup

[Mike Christie]

nbd requires socket families to support the shutdown method so the nbd
recv workqueue can be woken up from its sock_recvmsg call. If the socket
does not support the callout we will leave recv works running or get hangs
later when the device or module is removed.

This adds a check during socket connection/reconnection to make sure the
socket being passed in supports the needed callout.

Reported-by: syzbot+24c12fa8d218ed26011a@syzkaller.appspotmail.com
Fixes: e9e006f5fcf2 ("nbd: fix max number of supported devs")
Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/block/nbd.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 478aa86fc1f2..7bd9e92f6bb7 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -972,6 +972,25 @@ 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 (sock->ops->shutdown == sock_no_shutdown) {
+		dev_err(disk_to_dev(nbd->disk), "Unsupported socket: shutdown callout must be supported.\n");
+		*err = -EINVAL;
+		return NULL;
+	}
+
+	return sock;
+}
+
 static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
 			  bool netlink)
 {
@@ -981,7 +1000,7 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
 	struct nbd_sock *nsock;
 	int err;
 
-	sock = sockfd_lookup(arg, &err);
+	sock = nbd_get_socket(nbd, arg, &err);
 	if (!sock)
 		return err;
 
@@ -1033,7 +1052,7 @@ static int nbd_reconnect_socket(struct nbd_device *nbd, unsigned long arg)
 	int i;
 	int err;
 
-	sock = sockfd_lookup(arg, &err);
+	sock = nbd_get_socket(nbd, arg, &err);
 	if (!sock)
 		return err;
 
-- 
2.20.1

Re: [PATCH] nbd: verify socket is supported during setup

[Richard W.M. Jones]

On Thu, Oct 17, 2019 at 04:27:34PM -0500, Mike Christie wrote:
> nbd requires socket families to support the shutdown method so the nbd
> recv workqueue can be woken up from its sock_recvmsg call. If the socket
> does not support the callout we will leave recv works running or get hangs
> later when the device or module is removed.
> 
> This adds a check during socket connection/reconnection to make sure the
> socket being passed in supports the needed callout.
> 
> Reported-by: syzbot+24c12fa8d218ed26011a@syzkaller.appspotmail.com
> Fixes: e9e006f5fcf2 ("nbd: fix max number of supported devs")
> Signed-off-by: Mike Christie <mchristi@redhat.com>

I tested this patch to try to make sure it doesn't break typical
existing use cases of the kernel NBD client using Unix and TCP
servers.  I used a Fedora 30 virtual machine, nbdkit-1.12.8-1.fc30,
and the upstream kernel + your patch for testing.  I tested it using
loop-style commands similar to what I did in this talk[1] using
commands from the nbdkit manual[2].

Note I did *not* test the negative case, eg. NBD over a netlink
socket, as I have no easy way to test that.

Anyway, I can confirm that both Unix domain sockets and TCP to
localhost works fine with this patch, so:

Tested-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

[1] https://archive.fosdem.org/2019/schedule/event/nbdkit/
[2] http://libguestfs.org/nbdkit-loop.1.html

> ---
>  drivers/block/nbd.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 478aa86fc1f2..7bd9e92f6bb7 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -972,6 +972,25 @@ 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 (sock->ops->shutdown == sock_no_shutdown) {
> +		dev_err(disk_to_dev(nbd->disk), "Unsupported socket: shutdown callout must be supported.\n");
> +		*err = -EINVAL;
> +		return NULL;
> +	}
> +
> +	return sock;
> +}
> +
>  static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
>  			  bool netlink)
>  {
> @@ -981,7 +1000,7 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
>  	struct nbd_sock *nsock;
>  	int err;
>  
> -	sock = sockfd_lookup(arg, &err);
> +	sock = nbd_get_socket(nbd, arg, &err);
>  	if (!sock)
>  		return err;
>  
> @@ -1033,7 +1052,7 @@ static int nbd_reconnect_socket(struct nbd_device *nbd, unsigned long arg)
>  	int i;
>  	int err;
>  
> -	sock = sockfd_lookup(arg, &err);
> +	sock = nbd_get_socket(nbd, arg, &err);
>  	if (!sock)
>  		return err;
>  
> -- 
> 2.20.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [PATCH] nbd: verify socket is supported during setup

[Jens Axboe]

On 10/17/19 3:27 PM, Mike Christie wrote:
> nbd requires socket families to support the shutdown method so the nbd
> recv workqueue can be woken up from its sock_recvmsg call. If the socket
> does not support the callout we will leave recv works running or get hangs
> later when the device or module is removed.
> 
> This adds a check during socket connection/reconnection to make sure the
> socket being passed in supports the needed callout.

Applied, thanks Mike.

-- 
Jens Axboe