[PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Stefan Hajnoczi]

socket_get_fd() fails with the error "socket_get_fd: too many
connections" if the given listen backlog value is not 1.

Not all callers set the backlog to 1. For example, commit
582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
socket listen() backlog") uses SOMAXCONN. This will always fail with in
socket_get_fd().

This patch calls listen(2) on the fd to update the backlog value. The
socket may already be in the listen state. I have tested that this works
on Linux 5.10 and macOS Catalina.

As a bonus this allows us to detect when the fd cannot listen. Now we'll
be able to catch unbound or connected fds in socket_listen().

Drop the num argument from socket_get_fd() since this function is also
called by socket_connect() where a listen backlog value does not make
sense.

Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
Reported-by: Richard W.M. Jones <rjones@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8af0278f15..2463c49773 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1116,14 +1116,10 @@ fail:
     return NULL;
 }
 
-static int socket_get_fd(const char *fdstr, int num, Error **errp)
+static int socket_get_fd(const char *fdstr, Error **errp)
 {
     Monitor *cur_mon = monitor_cur();
     int fd;
-    if (num != 1) {
-        error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");
-        return -1;
-    }
     if (cur_mon) {
         fd = monitor_get_fd(cur_mon, fdstr, errp);
         if (fd < 0) {
@@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp)
         break;
 
     case SOCKET_ADDRESS_TYPE_FD:
-        fd = socket_get_fd(addr->u.fd.str, 1, errp);
+        fd = socket_get_fd(addr->u.fd.str, errp);
         break;
 
     case SOCKET_ADDRESS_TYPE_VSOCK:
@@ -1187,7 +1183,26 @@ int socket_listen(SocketAddress *addr, int num, Error **errp)
         break;
 
     case SOCKET_ADDRESS_TYPE_FD:
-        fd = socket_get_fd(addr->u.fd.str, num, errp);
+        fd = socket_get_fd(addr->u.fd.str, errp);
+        if (fd < 0) {
+            return -1;
+        }
+
+        /*
+         * If the socket is not yet in the listen state, then transition it to
+         * the listen state now.
+         *
+         * If it's already listening then this updates the backlog value as
+         * requested.
+         *
+         * If this socket cannot listen because it's already in another state
+         * (e.g. unbound or connected) then we'll catch the error here.
+         */
+        if (listen(fd, num) != 0) {
+            error_setg_errno(errp, errno, "Failed to listen on fd socket");
+            closesocket(fd);
+            return -1;
+        }
         break;
 
     case SOCKET_ADDRESS_TYPE_VSOCK:
-- 
2.29.2

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Richard W.M. Jones]

On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
> socket_get_fd() fails with the error "socket_get_fd: too many
> connections" if the given listen backlog value is not 1.
> 
> Not all callers set the backlog to 1. For example, commit
> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> socket_get_fd().
> 
> This patch calls listen(2) on the fd to update the backlog value. The
> socket may already be in the listen state. I have tested that this works
> on Linux 5.10 and macOS Catalina.
> 
> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> be able to catch unbound or connected fds in socket_listen().
> 
> Drop the num argument from socket_get_fd() since this function is also
> called by socket_connect() where a listen backlog value does not make
> sense.
> 
> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

I can confirm it fixes the problem found by the libnbd interop test, so:

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

Rich.

> ---
>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8af0278f15..2463c49773 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1116,14 +1116,10 @@ fail:
>      return NULL;
>  }
>  
> -static int socket_get_fd(const char *fdstr, int num, Error **errp)
> +static int socket_get_fd(const char *fdstr, Error **errp)
>  {
>      Monitor *cur_mon = monitor_cur();
>      int fd;
> -    if (num != 1) {
> -        error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");
> -        return -1;
> -    }
>      if (cur_mon) {
>          fd = monitor_get_fd(cur_mon, fdstr, errp);
>          if (fd < 0) {
> @@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp)
>          break;
>  
>      case SOCKET_ADDRESS_TYPE_FD:
> -        fd = socket_get_fd(addr->u.fd.str, 1, errp);
> +        fd = socket_get_fd(addr->u.fd.str, errp);
>          break;
>  
>      case SOCKET_ADDRESS_TYPE_VSOCK:
> @@ -1187,7 +1183,26 @@ int socket_listen(SocketAddress *addr, int num, Error **errp)
>          break;
>  
>      case SOCKET_ADDRESS_TYPE_FD:
> -        fd = socket_get_fd(addr->u.fd.str, num, errp);
> +        fd = socket_get_fd(addr->u.fd.str, errp);
> +        if (fd < 0) {
> +            return -1;
> +        }
> +
> +        /*
> +         * If the socket is not yet in the listen state, then transition it to
> +         * the listen state now.
> +         *
> +         * If it's already listening then this updates the backlog value as
> +         * requested.
> +         *
> +         * If this socket cannot listen because it's already in another state
> +         * (e.g. unbound or connected) then we'll catch the error here.
> +         */
> +        if (listen(fd, num) != 0) {
> +            error_setg_errno(errp, errno, "Failed to listen on fd socket");
> +            closesocket(fd);
> +            return -1;
> +        }
>          break;
>  
>      case SOCKET_ADDRESS_TYPE_VSOCK:
> -- 
> 2.29.2
> 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Eric Blake]

On 3/10/21 11:30 AM, Stefan Hajnoczi wrote:
> socket_get_fd() fails with the error "socket_get_fd: too many
> connections" if the given listen backlog value is not 1.
> 
> Not all callers set the backlog to 1. For example, commit
> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> socket_get_fd().
> 
> This patch calls listen(2) on the fd to update the backlog value. The
> socket may already be in the listen state. I have tested that this works
> on Linux 5.10 and macOS Catalina.
> 
> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> be able to catch unbound or connected fds in socket_listen().
> 
> Drop the num argument from socket_get_fd() since this function is also
> called by socket_connect() where a listen backlog value does not make
> sense.
> 
> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Stefano Garzarella]

On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
>socket_get_fd() fails with the error "socket_get_fd: too many
>connections" if the given listen backlog value is not 1.
>
>Not all callers set the backlog to 1. For example, commit
>582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
>socket listen() backlog") uses SOMAXCONN. This will always fail with in
>socket_get_fd().
>
>This patch calls listen(2) on the fd to update the backlog value. The
>socket may already be in the listen state. I have tested that this works
>on Linux 5.10 and macOS Catalina.

Confirmed also from the listen(2) man page of FreeBSD:
"A subsequent listen() system call on the listening socket allows the 
caller to change the maximum queue length using a new backlog argument."

>
>As a bonus this allows us to detect when the fd cannot listen. Now we'll
>be able to catch unbound or connected fds in socket_listen().
>
>Drop the num argument from socket_get_fd() since this function is also
>called by socket_connect() where a listen backlog value does not make
>sense.
>
>Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
>Reported-by: Richard W.M. Jones <rjones@redhat.com>
>Cc: Juan Quintela <quintela@redhat.com>
>Cc: Eric Blake <eblake@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3445 bytes --]

On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
> socket_get_fd() fails with the error "socket_get_fd: too many
> connections" if the given listen backlog value is not 1.
> 
> Not all callers set the backlog to 1. For example, commit
> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> socket_get_fd().
> 
> This patch calls listen(2) on the fd to update the backlog value. The
> socket may already be in the listen state. I have tested that this works
> on Linux 5.10 and macOS Catalina.
> 
> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> be able to catch unbound or connected fds in socket_listen().
> 
> Drop the num argument from socket_get_fd() since this function is also
> called by socket_connect() where a listen backlog value does not make
> sense.
> 
> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)

Dan and Gerd: Can this go via one of your trees?

Thanks,
Stefan

> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8af0278f15..2463c49773 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1116,14 +1116,10 @@ fail:
>      return NULL;
>  }
>  
> -static int socket_get_fd(const char *fdstr, int num, Error **errp)
> +static int socket_get_fd(const char *fdstr, Error **errp)
>  {
>      Monitor *cur_mon = monitor_cur();
>      int fd;
> -    if (num != 1) {
> -        error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");
> -        return -1;
> -    }
>      if (cur_mon) {
>          fd = monitor_get_fd(cur_mon, fdstr, errp);
>          if (fd < 0) {
> @@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp)
>          break;
>  
>      case SOCKET_ADDRESS_TYPE_FD:
> -        fd = socket_get_fd(addr->u.fd.str, 1, errp);
> +        fd = socket_get_fd(addr->u.fd.str, errp);
>          break;
>  
>      case SOCKET_ADDRESS_TYPE_VSOCK:
> @@ -1187,7 +1183,26 @@ int socket_listen(SocketAddress *addr, int num, Error **errp)
>          break;
>  
>      case SOCKET_ADDRESS_TYPE_FD:
> -        fd = socket_get_fd(addr->u.fd.str, num, errp);
> +        fd = socket_get_fd(addr->u.fd.str, errp);
> +        if (fd < 0) {
> +            return -1;
> +        }
> +
> +        /*
> +         * If the socket is not yet in the listen state, then transition it to
> +         * the listen state now.
> +         *
> +         * If it's already listening then this updates the backlog value as
> +         * requested.
> +         *
> +         * If this socket cannot listen because it's already in another state
> +         * (e.g. unbound or connected) then we'll catch the error here.
> +         */
> +        if (listen(fd, num) != 0) {
> +            error_setg_errno(errp, errno, "Failed to listen on fd socket");
> +            closesocket(fd);
> +            return -1;
> +        }
>          break;
>  
>      case SOCKET_ADDRESS_TYPE_VSOCK:
> -- 
> 2.29.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Eric Blake]

On 3/16/21 4:10 AM, Stefan Hajnoczi wrote:
> On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
>> socket_get_fd() fails with the error "socket_get_fd: too many
>> connections" if the given listen backlog value is not 1.
>>
>> Not all callers set the backlog to 1. For example, commit
>> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
>> socket listen() backlog") uses SOMAXCONN. This will always fail with in
>> socket_get_fd().
>>
>> This patch calls listen(2) on the fd to update the backlog value. The
>> socket may already be in the listen state. I have tested that this works
>> on Linux 5.10 and macOS Catalina.
>>
>> As a bonus this allows us to detect when the fd cannot listen. Now we'll
>> be able to catch unbound or connected fds in socket_listen().
>>
>> Drop the num argument from socket_get_fd() since this function is also
>> called by socket_connect() where a listen backlog value does not make
>> sense.
>>
>> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
>> Reported-by: Richard W.M. Jones <rjones@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
>>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> Dan and Gerd: Can this go via one of your trees?
> 

As it showed up as a regression in qemu-nbd, I can also consider queuing
it in my NBD tree.  However, I claim it counts as a bug fix, so it is
fine for -rc1 even if it misses soft freeze.

I'm fine whichever maintainer takes this, although I've now flagged it
to go through an NBD pull request if it doesn't land elsewhere sooner.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote:
> On 3/16/21 4:10 AM, Stefan Hajnoczi wrote:
> > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
> >> socket_get_fd() fails with the error "socket_get_fd: too many
> >> connections" if the given listen backlog value is not 1.
> >>
> >> Not all callers set the backlog to 1. For example, commit
> >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> >> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> >> socket_get_fd().
> >>
> >> This patch calls listen(2) on the fd to update the backlog value. The
> >> socket may already be in the listen state. I have tested that this works
> >> on Linux 5.10 and macOS Catalina.
> >>
> >> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> >> be able to catch unbound or connected fds in socket_listen().
> >>
> >> Drop the num argument from socket_get_fd() since this function is also
> >> called by socket_connect() where a listen backlog value does not make
> >> sense.
> >>
> >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> >> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Cc: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
> >>  1 file changed, 22 insertions(+), 7 deletions(-)
> > 
> > Dan and Gerd: Can this go via one of your trees?
> > 
> 
> As it showed up as a regression in qemu-nbd, I can also consider queuing
> it in my NBD tree.  However, I claim it counts as a bug fix, so it is
> fine for -rc1 even if it misses soft freeze.
> 
> I'm fine whichever maintainer takes this, although I've now flagged it
> to go through an NBD pull request if it doesn't land elsewhere sooner.

Thank you!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote:
> On 3/16/21 4:10 AM, Stefan Hajnoczi wrote:
> > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
> >> socket_get_fd() fails with the error "socket_get_fd: too many
> >> connections" if the given listen backlog value is not 1.
> >>
> >> Not all callers set the backlog to 1. For example, commit
> >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> >> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> >> socket_get_fd().
> >>
> >> This patch calls listen(2) on the fd to update the backlog value. The
> >> socket may already be in the listen state. I have tested that this works
> >> on Linux 5.10 and macOS Catalina.
> >>
> >> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> >> be able to catch unbound or connected fds in socket_listen().
> >>
> >> Drop the num argument from socket_get_fd() since this function is also
> >> called by socket_connect() where a listen backlog value does not make
> >> sense.
> >>
> >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> >> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Cc: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
> >>  1 file changed, 22 insertions(+), 7 deletions(-)
> > 
> > Dan and Gerd: Can this go via one of your trees?
> > 
> 
> As it showed up as a regression in qemu-nbd, I can also consider queuing
> it in my NBD tree.  However, I claim it counts as a bug fix, so it is
> fine for -rc1 even if it misses soft freeze.
> 
> I'm fine whichever maintainer takes this, although I've now flagged it
> to go through an NBD pull request if it doesn't land elsewhere sooner.

Hi Eric,
Is this patch going into QEMU 6.0?

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote:
> On 3/16/21 4:10 AM, Stefan Hajnoczi wrote:
> > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
> >> socket_get_fd() fails with the error "socket_get_fd: too many
> >> connections" if the given listen backlog value is not 1.
> >>
> >> Not all callers set the backlog to 1. For example, commit
> >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> >> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> >> socket_get_fd().
> >>
> >> This patch calls listen(2) on the fd to update the backlog value. The
> >> socket may already be in the listen state. I have tested that this works
> >> on Linux 5.10 and macOS Catalina.
> >>
> >> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> >> be able to catch unbound or connected fds in socket_listen().
> >>
> >> Drop the num argument from socket_get_fd() since this function is also
> >> called by socket_connect() where a listen backlog value does not make
> >> sense.
> >>
> >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> >> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Cc: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
> >>  1 file changed, 22 insertions(+), 7 deletions(-)
> > 
> > Dan and Gerd: Can this go via one of your trees?
> > 
> 
> As it showed up as a regression in qemu-nbd, I can also consider queuing
> it in my NBD tree.  However, I claim it counts as a bug fix, so it is
> fine for -rc1 even if it misses soft freeze.
> 
> I'm fine whichever maintainer takes this, although I've now flagged it
> to go through an NBD pull request if it doesn't land elsewhere sooner.

Ping? I didn't see this land in qemu.git.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Richard W.M. Jones]

On Tue, May 11, 2021 at 09:23:10AM +0100, Stefan Hajnoczi wrote:
> On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote:
> > On 3/16/21 4:10 AM, Stefan Hajnoczi wrote:
> > > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
> > >> socket_get_fd() fails with the error "socket_get_fd: too many
> > >> connections" if the given listen backlog value is not 1.
> > >>
> > >> Not all callers set the backlog to 1. For example, commit
> > >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> > >> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> > >> socket_get_fd().
> > >>
> > >> This patch calls listen(2) on the fd to update the backlog value. The
> > >> socket may already be in the listen state. I have tested that this works
> > >> on Linux 5.10 and macOS Catalina.
> > >>
> > >> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> > >> be able to catch unbound or connected fds in socket_listen().
> > >>
> > >> Drop the num argument from socket_get_fd() since this function is also
> > >> called by socket_connect() where a listen backlog value does not make
> > >> sense.
> > >>
> > >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> > >> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > >> Cc: Juan Quintela <quintela@redhat.com>
> > >> Cc: Eric Blake <eblake@redhat.com>
> > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > >> ---
> > >>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
> > >>  1 file changed, 22 insertions(+), 7 deletions(-)
> > > 
> > > Dan and Gerd: Can this go via one of your trees?
> > > 
> > 
> > As it showed up as a regression in qemu-nbd, I can also consider queuing
> > it in my NBD tree.  However, I claim it counts as a bug fix, so it is
> > fine for -rc1 even if it misses soft freeze.
> > 
> > I'm fine whichever maintainer takes this, although I've now flagged it
> > to go through an NBD pull request if it doesn't land elsewhere sooner.
> 
> Ping? I didn't see this land in qemu.git.

I notice this patch also fixes:

https://gitlab.com/qemu-project/qemu/-/issues/218

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Richard W.M. Jones]

On Tue, May 11, 2021 at 09:23:10AM +0100, Stefan Hajnoczi wrote:
> On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote:
> > On 3/16/21 4:10 AM, Stefan Hajnoczi wrote:
> > > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote:
> > >> socket_get_fd() fails with the error "socket_get_fd: too many
> > >> connections" if the given listen backlog value is not 1.
> > >>
> > >> Not all callers set the backlog to 1. For example, commit
> > >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> > >> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> > >> socket_get_fd().
> > >>
> > >> This patch calls listen(2) on the fd to update the backlog value. The
> > >> socket may already be in the listen state. I have tested that this works
> > >> on Linux 5.10 and macOS Catalina.
> > >>
> > >> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> > >> be able to catch unbound or connected fds in socket_listen().
> > >>
> > >> Drop the num argument from socket_get_fd() since this function is also
> > >> called by socket_connect() where a listen backlog value does not make
> > >> sense.
> > >>
> > >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> > >> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > >> Cc: Juan Quintela <quintela@redhat.com>
> > >> Cc: Eric Blake <eblake@redhat.com>
> > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > >> ---
> > >>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
> > >>  1 file changed, 22 insertions(+), 7 deletions(-)
> > > 
> > > Dan and Gerd: Can this go via one of your trees?
> > > 
> > 
> > As it showed up as a regression in qemu-nbd, I can also consider queuing
> > it in my NBD tree.  However, I claim it counts as a bug fix, so it is
> > fine for -rc1 even if it misses soft freeze.
> > 
> > I'm fine whichever maintainer takes this, although I've now flagged it
> > to go through an NBD pull request if it doesn't land elsewhere sooner.
> 
> Ping? I didn't see this land in qemu.git.

And a second reminder.  qemu-storage-daemon is broken in the released
qemu 6.0.0 so it'd be good to get this commit into the stable branch
as well.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [PATCH] sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog

[Stefan Hajnoczi]

Please consider this patch for the QEMU 6.0 stable release. The bug
was introduced in QEMU 6.0.

Thanks,
Stefan

On Wed, Mar 10, 2021 at 5:54 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> socket_get_fd() fails with the error "socket_get_fd: too many
> connections" if the given listen backlog value is not 1.
>
> Not all callers set the backlog to 1. For example, commit
> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for
> socket listen() backlog") uses SOMAXCONN. This will always fail with in
> socket_get_fd().
>
> This patch calls listen(2) on the fd to update the backlog value. The
> socket may already be in the listen state. I have tested that this works
> on Linux 5.10 and macOS Catalina.
>
> As a bonus this allows us to detect when the fd cannot listen. Now we'll
> be able to catch unbound or connected fds in socket_listen().
>
> Drop the num argument from socket_get_fd() since this function is also
> called by socket_connect() where a listen backlog value does not make
> sense.
>
> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen")
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/qemu-sockets.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8af0278f15..2463c49773 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1116,14 +1116,10 @@ fail:
>      return NULL;
>  }
>
> -static int socket_get_fd(const char *fdstr, int num, Error **errp)
> +static int socket_get_fd(const char *fdstr, Error **errp)
>  {
>      Monitor *cur_mon = monitor_cur();
>      int fd;
> -    if (num != 1) {
> -        error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");
> -        return -1;
> -    }
>      if (cur_mon) {
>          fd = monitor_get_fd(cur_mon, fdstr, errp);
>          if (fd < 0) {
> @@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp)
>          break;
>
>      case SOCKET_ADDRESS_TYPE_FD:
> -        fd = socket_get_fd(addr->u.fd.str, 1, errp);
> +        fd = socket_get_fd(addr->u.fd.str, errp);
>          break;
>
>      case SOCKET_ADDRESS_TYPE_VSOCK:
> @@ -1187,7 +1183,26 @@ int socket_listen(SocketAddress *addr, int num, Error **errp)
>          break;
>
>      case SOCKET_ADDRESS_TYPE_FD:
> -        fd = socket_get_fd(addr->u.fd.str, num, errp);
> +        fd = socket_get_fd(addr->u.fd.str, errp);
> +        if (fd < 0) {
> +            return -1;
> +        }
> +
> +        /*
> +         * If the socket is not yet in the listen state, then transition it to
> +         * the listen state now.
> +         *
> +         * If it's already listening then this updates the backlog value as
> +         * requested.
> +         *
> +         * If this socket cannot listen because it's already in another state
> +         * (e.g. unbound or connected) then we'll catch the error here.
> +         */
> +        if (listen(fd, num) != 0) {
> +            error_setg_errno(errp, errno, "Failed to listen on fd socket");
> +            closesocket(fd);
> +            return -1;
> +        }
>          break;
>
>      case SOCKET_ADDRESS_TYPE_VSOCK:
> --
> 2.29.2
>