* [PATCH 1/3] net: socket: prepare to cleanup net_init_socket()
2023-06-09 7:27 [PATCH 0/3] net: socket: do not close file descriptor if it's not a socket Laurent Vivier
@ 2023-06-09 7:27 ` Laurent Vivier
2023-06-15 5:06 ` David Gibson
2023-06-09 7:27 ` [PATCH 2/3] net: socket: move fd type checking to its own function Laurent Vivier
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Laurent Vivier @ 2023-06-09 7:27 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Jason Wang, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] net: socket: prepare to cleanup net_init_socket()
2023-06-09 7:27 ` [PATCH 1/3] net: socket: prepare to cleanup net_init_socket() Laurent Vivier
@ 2023-06-15 5:06 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2023-06-15 5:06 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, Jason Wang
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] net: socket: move fd type checking to its own function
2023-06-09 7:27 [PATCH 0/3] net: socket: do not close file descriptor if it's not a socket Laurent Vivier
2023-06-09 7:27 ` [PATCH 1/3] net: socket: prepare to cleanup net_init_socket() Laurent Vivier
@ 2023-06-09 7:27 ` Laurent Vivier
2023-06-15 5:09 ` David Gibson
2023-06-09 7:27 ` [PATCH 3/3] net: socket: remove net_init_socket() Laurent Vivier
2023-06-30 6:02 ` [PATCH 0/3] net: socket: do not close file descriptor if it's not a socket Jason Wang
3 siblings, 1 reply; 8+ messages in thread
From: Laurent Vivier @ 2023-06-09 7:27 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Jason Wang, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] net: socket: move fd type checking to its own function
2023-06-09 7:27 ` [PATCH 2/3] net: socket: move fd type checking to its own function Laurent Vivier
@ 2023-06-15 5:09 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2023-06-15 5:09 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, Jason Wang
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] net: socket: remove net_init_socket()
2023-06-09 7:27 [PATCH 0/3] net: socket: do not close file descriptor if it's not a socket Laurent Vivier
2023-06-09 7:27 ` [PATCH 1/3] net: socket: prepare to cleanup net_init_socket() Laurent Vivier
2023-06-09 7:27 ` [PATCH 2/3] net: socket: move fd type checking to its own function Laurent Vivier
@ 2023-06-09 7:27 ` Laurent Vivier
2023-06-15 5:10 ` David Gibson
2023-06-30 6:02 ` [PATCH 0/3] net: socket: do not close file descriptor if it's not a socket Jason Wang
3 siblings, 1 reply; 8+ messages in thread
From: Laurent Vivier @ 2023-06-09 7:27 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Jason Wang, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] net: socket: remove net_init_socket()
2023-06-09 7:27 ` [PATCH 3/3] net: socket: remove net_init_socket() Laurent Vivier
@ 2023-06-15 5:10 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2023-06-15 5:10 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, Jason Wang
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] net: socket: do not close file descriptor if it's not a socket
2023-06-09 7:27 [PATCH 0/3] net: socket: do not close file descriptor if it's not a socket Laurent Vivier
` (2 preceding siblings ...)
2023-06-09 7:27 ` [PATCH 3/3] net: socket: remove net_init_socket() Laurent Vivier
@ 2023-06-30 6:02 ` Jason Wang
3 siblings, 0 replies; 8+ messages in thread
From: Jason Wang @ 2023-06-30 6:02 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, David Gibson
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread