| Message ID | 20200630145737.232095-2-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: tap: check file descriptor can be used | expand |
On 6/30/20 4:57 PM, Laurent Vivier wrote: > qemu_set_nonblock() checks that the file descriptor can be used and, if > not, crashes QEMU. An assert() is used for that. The use of assert() is > used to detect programming error and the coredump will allow to debug > the problem. > > But in the case of the tap device, this assert() can be triggered by > a misconfiguration by the user. At startup, it's not a real problem, but it > can also happen during the hot-plug of a new device, and here it's a > problem because we can crash a perfectly healthy system. > > For instance: > # ip link add link virbr0 name macvtap0 type macvtap mode bridge > # ip link set macvtap0 up > # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) > # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP > (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 > (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 > (qemu) device_del net0 > (qemu) netdev_del hostnet0 > (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 > qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. > Aborted (core dumped) > > To avoid that, check the file descriptor is valid before passing it to > qemu_set_non_block() for "fd=" and "fds=" parameters. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/qemu/sockets.h | 1 + > net/tap.c | 13 +++++++++++++ > util/oslib-posix.c | 5 +++++ > util/oslib-win32.c | 6 ++++++ > 4 files changed, 25 insertions(+) > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h > index 57cd049d6edd..5b0c2d77ddad 100644 > --- a/include/qemu/sockets.h > +++ b/include/qemu/sockets.h > @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); > int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); > int socket_set_cork(int fd, int v); > int socket_set_nodelay(int fd); > +bool qemu_fd_is_valid(int fd); > void qemu_set_block(int fd); > void qemu_set_nonblock(int fd); > int socket_set_fast_reuse(int fd); > diff --git a/net/tap.c b/net/tap.c > index 6207f61f84ab..f65966aaccd8 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > return -1; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + return -1; > + } > + > qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, > goto free_fail; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + ret = -1; > + goto free_fail; > + } > + > qemu_set_nonblock(fd); > > if (i == 0) { > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 916f1be2243a..8d5705f598d3 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) > qemu_ram_munmap(-1, ptr, size); > } > > +bool qemu_fd_is_valid(int fd) > +{ > + return fcntl(fd, F_GETFL) != -1; > +} > + > void qemu_set_block(int fd) > { > int f; > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index e9b14ab17847..a6be9445cfdb 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) > } > #endif /* CONFIG_LOCALTIME_R */ > > +bool qemu_fd_is_valid(int fd) > +{ > + /* FIXME: how to check if fd is valid? */ > + return true; > +} > + > void qemu_set_block(int fd) > { > unsigned long opt = 0; >
On Tue, 30 Jun 2020 16:57:36 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > qemu_set_nonblock() checks that the file descriptor can be used and, if > not, crashes QEMU. An assert() is used for that. The use of assert() is > used to detect programming error and the coredump will allow to debug > the problem. > > But in the case of the tap device, this assert() can be triggered by > a misconfiguration by the user. At startup, it's not a real problem, but it > can also happen during the hot-plug of a new device, and here it's a > problem because we can crash a perfectly healthy system. > > For instance: > # ip link add link virbr0 name macvtap0 type macvtap mode bridge > # ip link set macvtap0 up > # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) > # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP > (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 > (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 > (qemu) device_del net0 > (qemu) netdev_del hostnet0 > (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 > qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. > Aborted (core dumped) > > To avoid that, check the file descriptor is valid before passing it to > qemu_set_non_block() for "fd=" and "fds=" parameters. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > include/qemu/sockets.h | 1 + > net/tap.c | 13 +++++++++++++ > util/oslib-posix.c | 5 +++++ > util/oslib-win32.c | 6 ++++++ > 4 files changed, 25 insertions(+) > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h > index 57cd049d6edd..5b0c2d77ddad 100644 > --- a/include/qemu/sockets.h > +++ b/include/qemu/sockets.h > @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); > int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); > int socket_set_cork(int fd, int v); > int socket_set_nodelay(int fd); > +bool qemu_fd_is_valid(int fd); > void qemu_set_block(int fd); > void qemu_set_nonblock(int fd); > int socket_set_fast_reuse(int fd); > diff --git a/net/tap.c b/net/tap.c > index 6207f61f84ab..f65966aaccd8 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > return -1; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + return -1; > + } > + > qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, > goto free_fail; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + ret = -1; > + goto free_fail; > + } > + > qemu_set_nonblock(fd); > > if (i == 0) { > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 916f1be2243a..8d5705f598d3 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) > qemu_ram_munmap(-1, ptr, size); > } > > +bool qemu_fd_is_valid(int fd) > +{ > + return fcntl(fd, F_GETFL) != -1; > +} > + > void qemu_set_block(int fd) > { > int f; > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index e9b14ab17847..a6be9445cfdb 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) > } > #endif /* CONFIG_LOCALTIME_R */ > > +bool qemu_fd_is_valid(int fd) > +{ > + /* FIXME: how to check if fd is valid? */ > + return true; > +} > + Maybe the following ? bool qemu_fd_is_valid(int fd) { return _get_osfhandle(fd) != INVALID_HANDLE_VALUE; } https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=vs-2019 Anyway, Reviewed-by: Greg Kurz <groug@kaod.org> > void qemu_set_block(int fd) > { > unsigned long opt = 0;
On 30/06/2020 18:06, Greg Kurz wrote: > On Tue, 30 Jun 2020 16:57:36 +0200 > Laurent Vivier <lvivier@redhat.com> wrote: > >> qemu_set_nonblock() checks that the file descriptor can be used and, if >> not, crashes QEMU. An assert() is used for that. The use of assert() is >> used to detect programming error and the coredump will allow to debug >> the problem. >> >> But in the case of the tap device, this assert() can be triggered by >> a misconfiguration by the user. At startup, it's not a real problem, but it >> can also happen during the hot-plug of a new device, and here it's a >> problem because we can crash a perfectly healthy system. >> >> For instance: >> # ip link add link virbr0 name macvtap0 type macvtap mode bridge >> # ip link set macvtap0 up >> # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) >> # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP >> (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 >> (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 >> (qemu) device_del net0 >> (qemu) netdev_del hostnet0 >> (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 >> qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. >> Aborted (core dumped) >> >> To avoid that, check the file descriptor is valid before passing it to >> qemu_set_non_block() for "fd=" and "fds=" parameters. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> include/qemu/sockets.h | 1 + >> net/tap.c | 13 +++++++++++++ >> util/oslib-posix.c | 5 +++++ >> util/oslib-win32.c | 6 ++++++ >> 4 files changed, 25 insertions(+) >> >> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h >> index 57cd049d6edd..5b0c2d77ddad 100644 >> --- a/include/qemu/sockets.h >> +++ b/include/qemu/sockets.h >> @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); >> int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); >> int socket_set_cork(int fd, int v); >> int socket_set_nodelay(int fd); >> +bool qemu_fd_is_valid(int fd); >> void qemu_set_block(int fd); >> void qemu_set_nonblock(int fd); >> int socket_set_fast_reuse(int fd); >> diff --git a/net/tap.c b/net/tap.c >> index 6207f61f84ab..f65966aaccd8 100644 >> --- a/net/tap.c >> +++ b/net/tap.c >> @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, >> return -1; >> } >> >> + /* Check if fd is valid */ >> + if (!qemu_fd_is_valid(fd)) { >> + error_setg(errp, "Invalid file descriptor %d", fd); >> + return -1; >> + } >> + >> qemu_set_nonblock(fd); >> >> vnet_hdr = tap_probe_vnet_hdr(fd); >> @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, >> goto free_fail; >> } >> >> + /* Check if fd is valid */ >> + if (!qemu_fd_is_valid(fd)) { >> + error_setg(errp, "Invalid file descriptor %d", fd); >> + ret = -1; >> + goto free_fail; >> + } >> + >> qemu_set_nonblock(fd); >> >> if (i == 0) { >> diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> index 916f1be2243a..8d5705f598d3 100644 >> --- a/util/oslib-posix.c >> +++ b/util/oslib-posix.c >> @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) >> qemu_ram_munmap(-1, ptr, size); >> } >> >> +bool qemu_fd_is_valid(int fd) >> +{ >> + return fcntl(fd, F_GETFL) != -1; >> +} >> + >> void qemu_set_block(int fd) >> { >> int f; >> diff --git a/util/oslib-win32.c b/util/oslib-win32.c >> index e9b14ab17847..a6be9445cfdb 100644 >> --- a/util/oslib-win32.c >> +++ b/util/oslib-win32.c >> @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) >> } >> #endif /* CONFIG_LOCALTIME_R */ >> >> +bool qemu_fd_is_valid(int fd) >> +{ >> + /* FIXME: how to check if fd is valid? */ >> + return true; >> +} >> + > > Maybe the following ? > > bool qemu_fd_is_valid(int fd) > { > return _get_osfhandle(fd) != INVALID_HANDLE_VALUE; > } > > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=vs-2019 For the v1, Philippe proposed: bool qemu_fd_is_valid(int fd) { unsigned long res; /* ignored */ return ioctlsocket(fd, FIONREAD, &res) == NO_ERROR; } https://docs.microsoft.com/en-us/windows/win32/winsock/winsock-ioctls The problem is I can't test. So I let the change to someone that will need it, fix it and test it... Thanks, Laurent
Laurent Vivier <lvivier@redhat.com> writes: > qemu_set_nonblock() checks that the file descriptor can be used and, if > not, crashes QEMU. An assert() is used for that. The use of assert() is > used to detect programming error and the coredump will allow to debug > the problem. > > But in the case of the tap device, this assert() can be triggered by > a misconfiguration by the user. At startup, it's not a real problem, but it > can also happen during the hot-plug of a new device, and here it's a > problem because we can crash a perfectly healthy system. > > For instance: > # ip link add link virbr0 name macvtap0 type macvtap mode bridge > # ip link set macvtap0 up > # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) > # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP > (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 > (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 > (qemu) device_del net0 > (qemu) netdev_del hostnet0 > (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 > qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. > Aborted (core dumped) > > To avoid that, check the file descriptor is valid before passing it to > qemu_set_non_block() for "fd=" and "fds=" parameters. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > include/qemu/sockets.h | 1 + > net/tap.c | 13 +++++++++++++ > util/oslib-posix.c | 5 +++++ > util/oslib-win32.c | 6 ++++++ > 4 files changed, 25 insertions(+) > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h > index 57cd049d6edd..5b0c2d77ddad 100644 > --- a/include/qemu/sockets.h > +++ b/include/qemu/sockets.h > @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); > int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); > int socket_set_cork(int fd, int v); > int socket_set_nodelay(int fd); > +bool qemu_fd_is_valid(int fd); > void qemu_set_block(int fd); > void qemu_set_nonblock(int fd); > int socket_set_fast_reuse(int fd); > diff --git a/net/tap.c b/net/tap.c > index 6207f61f84ab..f65966aaccd8 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > return -1; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + return -1; > + } > + > qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, > goto free_fail; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + ret = -1; > + goto free_fail; > + } > + > qemu_set_nonblock(fd); > > if (i == 0) { > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 916f1be2243a..8d5705f598d3 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) > qemu_ram_munmap(-1, ptr, size); > } > > +bool qemu_fd_is_valid(int fd) > +{ > + return fcntl(fd, F_GETFL) != -1; > +} > + > void qemu_set_block(int fd) > { > int f; > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index e9b14ab17847..a6be9445cfdb 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) > } > #endif /* CONFIG_LOCALTIME_R */ > > +bool qemu_fd_is_valid(int fd) > +{ > + /* FIXME: how to check if fd is valid? */ Please explain the FIXME's impact in the commit message. > + return true; > +} > + > void qemu_set_block(int fd) > { > unsigned long opt = 0; This patch is okay as it is. But I'd solve the problem differently, avoiding the FIXME: have qemu_try_set_block() return -errno on failure, have qemu_set_block() wrap around it and assert it succeeds. Then in net_init_tap(): ret = qemu_try_set_block(fd); if (ret < 0) { error_setg_errno(errp, -ret, "Can't use file descriptor %d", fd); } When @fd is bad (say -1), @ret is set to EBADF, and the error message looks like Can't use file descriptor -1: Bad file descriptor You should of course test the error message to see whether it makes sense even with a complex command line. If it doesn't, having it mention @name could perhaps help.
On 01/07/2020 07:50, Markus Armbruster wrote: > Laurent Vivier <lvivier@redhat.com> writes: > >> qemu_set_nonblock() checks that the file descriptor can be used and, if >> not, crashes QEMU. An assert() is used for that. The use of assert() is >> used to detect programming error and the coredump will allow to debug >> the problem. >> >> But in the case of the tap device, this assert() can be triggered by >> a misconfiguration by the user. At startup, it's not a real problem, but it >> can also happen during the hot-plug of a new device, and here it's a >> problem because we can crash a perfectly healthy system. >> >> For instance: >> # ip link add link virbr0 name macvtap0 type macvtap mode bridge >> # ip link set macvtap0 up >> # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) >> # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP >> (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 >> (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 >> (qemu) device_del net0 >> (qemu) netdev_del hostnet0 >> (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 >> qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. >> Aborted (core dumped) >> >> To avoid that, check the file descriptor is valid before passing it to >> qemu_set_non_block() for "fd=" and "fds=" parameters. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> include/qemu/sockets.h | 1 + >> net/tap.c | 13 +++++++++++++ >> util/oslib-posix.c | 5 +++++ >> util/oslib-win32.c | 6 ++++++ >> 4 files changed, 25 insertions(+) >> >> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h >> index 57cd049d6edd..5b0c2d77ddad 100644 >> --- a/include/qemu/sockets.h >> +++ b/include/qemu/sockets.h >> @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); >> int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); >> int socket_set_cork(int fd, int v); >> int socket_set_nodelay(int fd); >> +bool qemu_fd_is_valid(int fd); >> void qemu_set_block(int fd); >> void qemu_set_nonblock(int fd); >> int socket_set_fast_reuse(int fd); >> diff --git a/net/tap.c b/net/tap.c >> index 6207f61f84ab..f65966aaccd8 100644 >> --- a/net/tap.c >> +++ b/net/tap.c >> @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, >> return -1; >> } >> >> + /* Check if fd is valid */ >> + if (!qemu_fd_is_valid(fd)) { >> + error_setg(errp, "Invalid file descriptor %d", fd); >> + return -1; >> + } >> + >> qemu_set_nonblock(fd); >> >> vnet_hdr = tap_probe_vnet_hdr(fd); >> @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, >> goto free_fail; >> } >> >> + /* Check if fd is valid */ >> + if (!qemu_fd_is_valid(fd)) { >> + error_setg(errp, "Invalid file descriptor %d", fd); >> + ret = -1; >> + goto free_fail; >> + } >> + >> qemu_set_nonblock(fd); >> >> if (i == 0) { >> diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> index 916f1be2243a..8d5705f598d3 100644 >> --- a/util/oslib-posix.c >> +++ b/util/oslib-posix.c >> @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) >> qemu_ram_munmap(-1, ptr, size); >> } >> >> +bool qemu_fd_is_valid(int fd) >> +{ >> + return fcntl(fd, F_GETFL) != -1; >> +} >> + >> void qemu_set_block(int fd) >> { >> int f; >> diff --git a/util/oslib-win32.c b/util/oslib-win32.c >> index e9b14ab17847..a6be9445cfdb 100644 >> --- a/util/oslib-win32.c >> +++ b/util/oslib-win32.c >> @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) >> } >> #endif /* CONFIG_LOCALTIME_R */ >> >> +bool qemu_fd_is_valid(int fd) >> +{ >> + /* FIXME: how to check if fd is valid? */ > > Please explain the FIXME's impact in the commit message. > >> + return true; >> +} >> + >> void qemu_set_block(int fd) >> { >> unsigned long opt = 0; > > This patch is okay as it is. But I'd solve the problem differently, > avoiding the FIXME: have qemu_try_set_block() return -errno on failure, > have qemu_set_block() wrap around it and assert it succeeds. Then in > net_init_tap(): > > ret = qemu_try_set_block(fd); > if (ret < 0) { > error_setg_errno(errp, -ret, "Can't use file descriptor %d", > fd); > } > > When @fd is bad (say -1), @ret is set to EBADF, and the error message > looks like > > Can't use file descriptor -1: Bad file descriptor > > You should of course test the error message to see whether it makes > sense even with a complex command line. If it doesn't, having > it mention @name could perhaps help. > Yes, it's a good idea. I'm going to do that. Thanks, Laurent
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h index 57cd049d6edd..5b0c2d77ddad 100644 --- a/include/qemu/sockets.h +++ b/include/qemu/sockets.h @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); int socket_set_cork(int fd, int v); int socket_set_nodelay(int fd); +bool qemu_fd_is_valid(int fd); void qemu_set_block(int fd); void qemu_set_nonblock(int fd); int socket_set_fast_reuse(int fd); diff --git a/net/tap.c b/net/tap.c index 6207f61f84ab..f65966aaccd8 100644 --- a/net/tap.c +++ b/net/tap.c @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, return -1; } + /* Check if fd is valid */ + if (!qemu_fd_is_valid(fd)) { + error_setg(errp, "Invalid file descriptor %d", fd); + return -1; + } + qemu_set_nonblock(fd); vnet_hdr = tap_probe_vnet_hdr(fd); @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, goto free_fail; } + /* Check if fd is valid */ + if (!qemu_fd_is_valid(fd)) { + error_setg(errp, "Invalid file descriptor %d", fd); + ret = -1; + goto free_fail; + } + qemu_set_nonblock(fd); if (i == 0) { diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 916f1be2243a..8d5705f598d3 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) qemu_ram_munmap(-1, ptr, size); } +bool qemu_fd_is_valid(int fd) +{ + return fcntl(fd, F_GETFL) != -1; +} + void qemu_set_block(int fd) { int f; diff --git a/util/oslib-win32.c b/util/oslib-win32.c index e9b14ab17847..a6be9445cfdb 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) } #endif /* CONFIG_LOCALTIME_R */ +bool qemu_fd_is_valid(int fd) +{ + /* FIXME: how to check if fd is valid? */ + return true; +} + void qemu_set_block(int fd) { unsigned long opt = 0;
qemu_set_nonblock() checks that the file descriptor can be used and, if not, crashes QEMU. An assert() is used for that. The use of assert() is used to detect programming error and the coredump will allow to debug the problem. But in the case of the tap device, this assert() can be triggered by a misconfiguration by the user. At startup, it's not a real problem, but it can also happen during the hot-plug of a new device, and here it's a problem because we can crash a perfectly healthy system. For instance: # ip link add link virbr0 name macvtap0 type macvtap mode bridge # ip link set macvtap0 up # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 (qemu) device_del net0 (qemu) netdev_del hostnet0 (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. Aborted (core dumped) To avoid that, check the file descriptor is valid before passing it to qemu_set_non_block() for "fd=" and "fds=" parameters. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- include/qemu/sockets.h | 1 + net/tap.c | 13 +++++++++++++ util/oslib-posix.c | 5 +++++ util/oslib-win32.c | 6 ++++++ 4 files changed, 25 insertions(+)