[PATCH 0/3] net: socket: do not close file descriptor if it's not a socket

[PATCH 0/3] net: socket: do not close file descriptor if it's not a socket

[Laurent Vivier]

The socket netdev with a file descriptor (fd) cannot be removed
and then added again because the fd is closed when the backend is
removed and thus is not available anymore when we want to add the
backend again.

But this can bring to a core dump:
1- boot a VM with an fd socket netdev
2- remove the netdev
3- reboot
4- add the netdev again, it fails because the fd is not a
   socket, and then closed
5- stop QEMU -> core dump

On reboot (step 3) the fd is allocated to another use in QEMU, and when
we try to use it with a socket netdev, it fails. But the netdev backend
closes the file descriptor that is in use by another part of QEMU.
We can see the core dump on QEMU exit because it tries to close
an invalid file descriptor.

It happens for instance when we have a PCI device and the fd is allocated
to a VirtIOIRQFD on reboot.

Moreover, using "netdev socket,fd=X" allows an user to close any QEMU
internal file descriptor from an HMP or QMP interface.

Laurent Vivier (3):
  net: socket: prepare to cleanup net_init_socket()
  net: socket: move fd type checking to its own function
  net: socket: remove net_init_socket()

 net/socket.c | 53 +++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

-- 
2.39.2

[PATCH 1/3] net: socket: prepare to cleanup net_init_socket()

[Laurent Vivier]

Use directly net_socket_fd_init_stream() and net_socket_fd_init_dgram()
when the socket type is already known.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 net/socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index ba6e5b0b0035..24dcaa55bc46 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -587,7 +587,7 @@ static int net_socket_connect_init(NetClientState *peer,
             break;
         }
     }
-    s = net_socket_fd_init(peer, model, name, fd, connected, NULL, errp);
+    s = net_socket_fd_init_stream(peer, model, name, fd, connected);
     if (!s) {
         return -1;
     }
@@ -629,7 +629,7 @@ static int net_socket_mcast_init(NetClientState *peer,
         return -1;
     }
 
-    s = net_socket_fd_init(peer, model, name, fd, 0, NULL, errp);
+    s = net_socket_fd_init_dgram(peer, model, name, fd, 0, NULL, errp);
     if (!s) {
         return -1;
     }
@@ -683,7 +683,7 @@ static int net_socket_udp_init(NetClientState *peer,
     }
     qemu_socket_set_nonblock(fd);
 
-    s = net_socket_fd_init(peer, model, name, fd, 0, NULL, errp);
+    s = net_socket_fd_init_dgram(peer, model, name, fd, 0, NULL, errp);
     if (!s) {
         return -1;
     }
-- 
2.39.2

[PATCH 2/3] net: socket: move fd type checking to its own function

[Laurent Vivier]

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 net/socket.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 24dcaa55bc46..6b1f0fec3a10 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -446,16 +446,32 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
     return s;
 }
 
+static int net_socket_fd_check(int fd, Error **errp)
+{
+    int so_type, optlen = sizeof(so_type);
+
+    if (getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
+        (socklen_t *)&optlen) < 0) {
+        error_setg(errp, "can't get socket option SO_TYPE");
+        return -1;
+    }
+    if (so_type != SOCK_DGRAM && so_type != SOCK_STREAM) {
+        error_setg(errp, "socket type=%d for fd=%d must be either"
+                   " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+        return -1;
+    }
+    return so_type;
+}
+
 static NetSocketState *net_socket_fd_init(NetClientState *peer,
                                           const char *model, const char *name,
                                           int fd, int is_connected,
                                           const char *mc, Error **errp)
 {
-    int so_type = -1, optlen=sizeof(so_type);
+    int so_type;
 
-    if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
-        (socklen_t *)&optlen)< 0) {
-        error_setg(errp, "can't get socket option SO_TYPE");
+    so_type = net_socket_fd_check(fd, errp);
+    if (so_type < 0) {
         close(fd);
         return NULL;
     }
@@ -465,10 +481,6 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
                                         mc, errp);
     case SOCK_STREAM:
         return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
-    default:
-        error_setg(errp, "socket type=%d for fd=%d must be either"
-                   " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
-        close(fd);
     }
     return NULL;
 }
-- 
2.39.2

[PATCH 3/3] net: socket: remove net_init_socket()

[Laurent Vivier]

Move the file descriptor type checking before doing anything with it.
If it's not usable, don't close it as it could be in use by another
part of QEMU, only fail and report an error.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 net/socket.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 6b1f0fec3a10..8e3702e1f3a8 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -463,28 +463,6 @@ static int net_socket_fd_check(int fd, Error **errp)
     return so_type;
 }
 
-static NetSocketState *net_socket_fd_init(NetClientState *peer,
-                                          const char *model, const char *name,
-                                          int fd, int is_connected,
-                                          const char *mc, Error **errp)
-{
-    int so_type;
-
-    so_type = net_socket_fd_check(fd, errp);
-    if (so_type < 0) {
-        close(fd);
-        return NULL;
-    }
-    switch(so_type) {
-    case SOCK_DGRAM:
-        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected,
-                                        mc, errp);
-    case SOCK_STREAM:
-        return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
-    }
-    return NULL;
-}
-
 static void net_socket_accept(void *opaque)
 {
     NetSocketState *s = opaque;
@@ -728,21 +706,34 @@ int net_init_socket(const Netdev *netdev, const char *name,
     }
 
     if (sock->fd) {
-        int fd, ret;
+        int fd, ret, so_type;
 
         fd = monitor_fd_param(monitor_cur(), sock->fd, errp);
         if (fd == -1) {
             return -1;
         }
+        so_type = net_socket_fd_check(fd, errp);
+        if (so_type < 0) {
+            return -1;
+        }
         ret = qemu_socket_try_set_nonblock(fd);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
                              name, fd);
             return -1;
         }
-        if (!net_socket_fd_init(peer, "socket", name, fd, 1, sock->mcast,
-                                errp)) {
-            return -1;
+        switch (so_type) {
+        case SOCK_DGRAM:
+            if (!net_socket_fd_init_dgram(peer, "socket", name, fd, 1,
+                                          sock->mcast, errp)) {
+                return -1;
+            }
+            break;
+        case SOCK_STREAM:
+            if (!net_socket_fd_init_stream(peer, "socket", name, fd, 1)) {
+                return -1;
+            }
+            break;
         }
         return 0;
     }
-- 
2.39.2

Re: [PATCH 1/3] net: socket: prepare to cleanup net_init_socket()

[David Gibson]

On Fri,  9 Jun 2023 09:27:46 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Use directly net_socket_fd_init_stream() and net_socket_fd_init_dgram()
> when the socket type is already known.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

This makes sense as a clean up regardless of the rest of the series.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>


> ---
>  net/socket.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index ba6e5b0b0035..24dcaa55bc46 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -587,7 +587,7 @@ static int net_socket_connect_init(NetClientState *peer,
>              break;
>          }
>      }
> -    s = net_socket_fd_init(peer, model, name, fd, connected, NULL, errp);
> +    s = net_socket_fd_init_stream(peer, model, name, fd, connected);
>      if (!s) {
>          return -1;
>      }
> @@ -629,7 +629,7 @@ static int net_socket_mcast_init(NetClientState *peer,
>          return -1;
>      }
>  
> -    s = net_socket_fd_init(peer, model, name, fd, 0, NULL, errp);
> +    s = net_socket_fd_init_dgram(peer, model, name, fd, 0, NULL, errp);
>      if (!s) {
>          return -1;
>      }
> @@ -683,7 +683,7 @@ static int net_socket_udp_init(NetClientState *peer,
>      }
>      qemu_socket_set_nonblock(fd);
>  
> -    s = net_socket_fd_init(peer, model, name, fd, 0, NULL, errp);
> +    s = net_socket_fd_init_dgram(peer, model, name, fd, 0, NULL, errp);
>      if (!s) {
>          return -1;
>      }
> -- 
> 2.39.2
> 


-- 
David Gibson <dgibson@redhat.com>
Principal Software Engineer, Virtualization, Red Hat

Re: [PATCH 2/3] net: socket: move fd type checking to its own function

[David Gibson]

On Fri,  9 Jun 2023 09:27:47 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>


> ---
>  net/socket.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 24dcaa55bc46..6b1f0fec3a10 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -446,16 +446,32 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>      return s;
>  }
>  
> +static int net_socket_fd_check(int fd, Error **errp)
> +{
> +    int so_type, optlen = sizeof(so_type);
> +
> +    if (getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
> +        (socklen_t *)&optlen) < 0) {
> +        error_setg(errp, "can't get socket option SO_TYPE");
> +        return -1;
> +    }
> +    if (so_type != SOCK_DGRAM && so_type != SOCK_STREAM) {
> +        error_setg(errp, "socket type=%d for fd=%d must be either"
> +                   " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
> +        return -1;
> +    }
> +    return so_type;
> +}
> +> 
>  static NetSocketState *net_socket_fd_init(NetClientState *peer,
>                                            const char *model, const char *name,
>                                            int fd, int is_connected,
>                                            const char *mc, Error **errp)
>  {
> -    int so_type = -1, optlen=sizeof(so_type);
> +    int so_type;
>  
> -    if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
> -        (socklen_t *)&optlen)< 0) {
> -        error_setg(errp, "can't get socket option SO_TYPE");
> +    so_type = net_socket_fd_check(fd, errp);
> +    if (so_type < 0) {
>          close(fd);
>          return NULL;
>      }
> @@ -465,10 +481,6 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
>                                          mc, errp);
>      case SOCK_STREAM:
>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> -    default:
> -        error_setg(errp, "socket type=%d for fd=%d must be either"
> -                   " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
> -        close(fd);
>      }
>      return NULL;
>  }
> -- 
> 2.39.2
> 


-- 
David Gibson <dgibson@redhat.com>
Principal Software Engineer, Virtualization, Red Hat

Re: [PATCH 3/3] net: socket: remove net_init_socket()

[David Gibson]

On Fri,  9 Jun 2023 09:27:48 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Move the file descriptor type checking before doing anything with it.
> If it's not usable, don't close it as it could be in use by another
> part of QEMU, only fail and report an error.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  net/socket.c | 43 +++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 6b1f0fec3a10..8e3702e1f3a8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -463,28 +463,6 @@ static int net_socket_fd_check(int fd, Error **errp)
>      return so_type;
>  }
>  
> -static NetSocketState *net_socket_fd_init(NetClientState *peer,
> -                                          const char *model, const char *name,
> -                                          int fd, int is_connected,
> -                                          const char *mc, Error **errp)
> -{
> -    int so_type;
> -
> -    so_type = net_socket_fd_check(fd, errp);
> -    if (so_type < 0) {
> -        close(fd);
> -        return NULL;
> -    }
> -    switch(so_type) {
> -    case SOCK_DGRAM:
> -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected,
> -                                        mc, errp);
> -    case SOCK_STREAM:
> -        return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> -    }
> -    return NULL;
> -}
> -
>  static void net_socket_accept(void *opaque)
>  {
>      NetSocketState *s = opaque;
> @@ -728,21 +706,34 @@ int net_init_socket(const Netdev *netdev, const char *name,
>      }
>  
>      if (sock->fd) {
> -        int fd, ret;
> +        int fd, ret, so_type;
>  
>          fd = monitor_fd_param(monitor_cur(), sock->fd, errp);
>          if (fd == -1) {
>              return -1;
>          }
> +        so_type = net_socket_fd_check(fd, errp);
> +        if (so_type < 0) {
> +            return -1;
> +        }
>          ret = qemu_socket_try_set_nonblock(fd);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
>                               name, fd);
>              return -1;
>          }
> -        if (!net_socket_fd_init(peer, "socket", name, fd, 1, sock->mcast,
> -                                errp)) {
> -            return -1;
> +        switch (so_type) {
> +        case SOCK_DGRAM:
> +            if (!net_socket_fd_init_dgram(peer, "socket", name, fd, 1,
> +                                          sock->mcast, errp)) {
> +                return -1;
> +            }
> +            break;
> +        case SOCK_STREAM:
> +            if (!net_socket_fd_init_stream(peer, "socket", name, fd, 1)) {
> +                return -1;
> +            }
> +            break;
>          }
>          return 0;
>      }
> -- 
> 2.39.2
> 


-- 
David Gibson <dgibson@redhat.com>
Principal Software Engineer, Virtualization, Red Hat

Re: [PATCH 0/3] net: socket: do not close file descriptor if it's not a socket

[Jason Wang]

On Fri, Jun 9, 2023 at 3:28 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> The socket netdev with a file descriptor (fd) cannot be removed
> and then added again because the fd is closed when the backend is
> removed and thus is not available anymore when we want to add the
> backend again.
>
> But this can bring to a core dump:
> 1- boot a VM with an fd socket netdev
> 2- remove the netdev
> 3- reboot
> 4- add the netdev again, it fails because the fd is not a
>    socket, and then closed
> 5- stop QEMU -> core dump
>
> On reboot (step 3) the fd is allocated to another use in QEMU, and when
> we try to use it with a socket netdev, it fails. But the netdev backend
> closes the file descriptor that is in use by another part of QEMU.
> We can see the core dump on QEMU exit because it tries to close
> an invalid file descriptor.
>
> It happens for instance when we have a PCI device and the fd is allocated
> to a VirtIOIRQFD on reboot.
>
> Moreover, using "netdev socket,fd=X" allows an user to close any QEMU
> internal file descriptor from an HMP or QMP interface.
>
> Laurent Vivier (3):
>   net: socket: prepare to cleanup net_init_socket()
>   net: socket: move fd type checking to its own function
>   net: socket: remove net_init_socket()
>
>  net/socket.c | 53 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 25 deletions(-)

Queued.

Thanks

>
> --
> 2.39.2
>
>