| Message ID | 20200701193951.36248-2-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: tap: check file descriptor can be used | expand |
On 7/1/20 9:39 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, add a function, qemu_try_set_nonblock(), that allows to report the > problem without crashing. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > include/qemu/sockets.h | 1 + > net/tap.c | 16 +++++++++--- > util/oslib-posix.c | 26 +++++++++++++------ > util/oslib-win32.c | 57 ++++++++++++++++++++++++------------------ > 4 files changed, 64 insertions(+), 36 deletions(-) > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h > index 57cd049d6edd..7d1f8135767d 100644 > --- a/include/qemu/sockets.h > +++ b/include/qemu/sockets.h > @@ -18,6 +18,7 @@ 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); > void qemu_set_block(int fd); > +int qemu_try_set_nonblock(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..fb04c9044ce2 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -766,6 +766,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > Error *err = NULL; > const char *vhostfdname; > char ifname[128]; > + int ret = 0; No need to zero-initialize. Otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > assert(netdev->type == NET_CLIENT_DRIVER_TAP); > tap = &netdev->u.tap; > @@ -795,7 +796,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > return -1; > } > > - qemu_set_nonblock(fd); > + ret = qemu_try_set_nonblock(fd); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d", > + name, fd); > + return -1; > + } > > vnet_hdr = tap_probe_vnet_hdr(fd); > > @@ -810,7 +816,6 @@ int net_init_tap(const Netdev *netdev, const char *name, > char **fds; > char **vhost_fds; > int nfds = 0, nvhosts = 0; > - int ret = 0; > > if (tap->has_ifname || tap->has_script || tap->has_downscript || > tap->has_vnet_hdr || tap->has_helper || tap->has_queues || > @@ -843,7 +848,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > goto free_fail; > } > > - qemu_set_nonblock(fd); > + ret = qemu_try_set_nonblock(fd); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d", > + name, fd); > + goto free_fail; > + } > > if (i == 0) { > vnet_hdr = tap_probe_vnet_hdr(fd); > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 916f1be2243a..149254cd691f 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -253,25 +253,35 @@ void qemu_set_block(int fd) > assert(f != -1); > } > > -void qemu_set_nonblock(int fd) > +int qemu_try_set_nonblock(int fd) > { > int f; > f = fcntl(fd, F_GETFL); > - assert(f != -1); > - f = fcntl(fd, F_SETFL, f | O_NONBLOCK); > -#ifdef __OpenBSD__ > if (f == -1) { > + return -errno; > + } > + if (fcntl(fd, F_SETFL, f | O_NONBLOCK) == -1) { > +#ifdef __OpenBSD__ > /* > * Previous to OpenBSD 6.3, fcntl(F_SETFL) is not permitted on > * memory devices and sets errno to ENODEV. > * It's OK if we fail to set O_NONBLOCK on devices like /dev/null, > * because they will never block anyway. > */ > - assert(errno == ENODEV); > - } > -#else > - assert(f != -1); > + if (errno == ENODEV) { > + return 0; > + } > #endif > + return -errno; > + } > + return 0; > +} > + > +void qemu_set_nonblock(int fd) > +{ > + int f; > + f = qemu_try_set_nonblock(fd); > + assert(f == 0); > } > > int socket_set_fast_reuse(int fd) > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index e9b14ab17847..5548ce6038f3 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -132,31 +132,6 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) > } > #endif /* CONFIG_LOCALTIME_R */ > > -void qemu_set_block(int fd) > -{ > - unsigned long opt = 0; > - WSAEventSelect(fd, NULL, 0); > - ioctlsocket(fd, FIONBIO, &opt); > -} > - > -void qemu_set_nonblock(int fd) > -{ > - unsigned long opt = 1; > - ioctlsocket(fd, FIONBIO, &opt); > - qemu_fd_register(fd); > -} > - > -int socket_set_fast_reuse(int fd) > -{ > - /* Enabling the reuse of an endpoint that was used by a socket still in > - * TIME_WAIT state is usually performed by setting SO_REUSEADDR. On Windows > - * fast reuse is the default and SO_REUSEADDR does strange things. So we > - * don't have to do anything here. More info can be found at: > - * http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx */ > - return 0; > -} > - > - > static int socket_error(void) > { > switch (WSAGetLastError()) { > @@ -233,6 +208,38 @@ static int socket_error(void) > } > } > > +void qemu_set_block(int fd) > +{ > + unsigned long opt = 0; > + WSAEventSelect(fd, NULL, 0); > + ioctlsocket(fd, FIONBIO, &opt); > +} > + > +int qemu_try_set_nonblock(int fd) > +{ > + unsigned long opt = 1; > + if (ioctlsocket(fd, FIONBIO, &opt) != NO_ERROR) { > + return -socket_error(); > + } > + qemu_fd_register(fd); > + return 0; > +} > + > +void qemu_set_nonblock(int fd) > +{ > + (void)qemu_try_set_nonblock(fd); > +} > + > +int socket_set_fast_reuse(int fd) > +{ > + /* Enabling the reuse of an endpoint that was used by a socket still in > + * TIME_WAIT state is usually performed by setting SO_REUSEADDR. On Windows > + * fast reuse is the default and SO_REUSEADDR does strange things. So we > + * don't have to do anything here. More info can be found at: > + * http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx */ > + return 0; > +} > + > int inet_aton(const char *cp, struct in_addr *ia) > { > uint32_t addr = inet_addr(cp); >
On 02/07/2020 10:00, Philippe Mathieu-Daudé wrote: > On 7/1/20 9:39 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, add a function, qemu_try_set_nonblock(), that allows to report the >> problem without crashing. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> include/qemu/sockets.h | 1 + >> net/tap.c | 16 +++++++++--- >> util/oslib-posix.c | 26 +++++++++++++------ >> util/oslib-win32.c | 57 ++++++++++++++++++++++++------------------ >> 4 files changed, 64 insertions(+), 36 deletions(-) >> >> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h >> index 57cd049d6edd..7d1f8135767d 100644 >> --- a/include/qemu/sockets.h >> +++ b/include/qemu/sockets.h >> @@ -18,6 +18,7 @@ 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); >> void qemu_set_block(int fd); >> +int qemu_try_set_nonblock(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..fb04c9044ce2 100644 >> --- a/net/tap.c >> +++ b/net/tap.c >> @@ -766,6 +766,7 @@ int net_init_tap(const Netdev *netdev, const char *name, >> Error *err = NULL; >> const char *vhostfdname; >> char ifname[128]; >> + int ret = 0; > > No need to zero-initialize. > > Otherwise: > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > I would agree with you but gcc doesn't: net/tap.c:769:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 769 | int ret; | ^~~ if nfds == 0, we don't enter in the loop and ret is never set. Thanks, Laurent
On 7/2/20 10:17 AM, Laurent Vivier wrote: > On 02/07/2020 10:00, Philippe Mathieu-Daudé wrote: >> On 7/1/20 9:39 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, add a function, qemu_try_set_nonblock(), that allows to report the >>> problem without crashing. >>> >>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>> --- >>> include/qemu/sockets.h | 1 + >>> net/tap.c | 16 +++++++++--- >>> util/oslib-posix.c | 26 +++++++++++++------ >>> util/oslib-win32.c | 57 ++++++++++++++++++++++++------------------ >>> 4 files changed, 64 insertions(+), 36 deletions(-) >>> >>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h >>> index 57cd049d6edd..7d1f8135767d 100644 >>> --- a/include/qemu/sockets.h >>> +++ b/include/qemu/sockets.h >>> @@ -18,6 +18,7 @@ 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); >>> void qemu_set_block(int fd); >>> +int qemu_try_set_nonblock(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..fb04c9044ce2 100644 >>> --- a/net/tap.c >>> +++ b/net/tap.c >>> @@ -766,6 +766,7 @@ int net_init_tap(const Netdev *netdev, const char *name, >>> Error *err = NULL; >>> const char *vhostfdname; >>> char ifname[128]; >>> + int ret = 0; >> >> No need to zero-initialize. >> >> Otherwise: >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> > > I would agree with you but gcc doesn't: > > net/tap.c:769:9: error: ‘ret’ may be used uninitialized in this function > [-Werror=maybe-uninitialized] > 769 | int ret; > | ^~~ I tried to outsmart GCC by manual review :/ > > if nfds == 0, we don't enter in the loop and ret is never set. > > Thanks, > Laurent >
On 7/2/20 10:18 AM, Philippe Mathieu-Daudé wrote: > On 7/2/20 10:17 AM, Laurent Vivier wrote: >> On 02/07/2020 10:00, Philippe Mathieu-Daudé wrote: >>> On 7/1/20 9:39 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, add a function, qemu_try_set_nonblock(), that allows to report the >>>> problem without crashing. >>>> >>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>>> --- >>>> include/qemu/sockets.h | 1 + >>>> net/tap.c | 16 +++++++++--- >>>> util/oslib-posix.c | 26 +++++++++++++------ >>>> util/oslib-win32.c | 57 ++++++++++++++++++++++++------------------ >>>> 4 files changed, 64 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h >>>> index 57cd049d6edd..7d1f8135767d 100644 >>>> --- a/include/qemu/sockets.h >>>> +++ b/include/qemu/sockets.h >>>> @@ -18,6 +18,7 @@ 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); >>>> void qemu_set_block(int fd); >>>> +int qemu_try_set_nonblock(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..fb04c9044ce2 100644 >>>> --- a/net/tap.c >>>> +++ b/net/tap.c >>>> @@ -766,6 +766,7 @@ int net_init_tap(const Netdev *netdev, const char *name, >>>> Error *err = NULL; >>>> const char *vhostfdname; >>>> char ifname[128]; >>>> + int ret = 0; >>> >>> No need to zero-initialize. >>> >>> Otherwise: >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> >> >> I would agree with you but gcc doesn't: >> >> net/tap.c:769:9: error: ‘ret’ may be used uninitialized in this function >> [-Werror=maybe-uninitialized] >> 769 | int ret; >> | ^~~ > > I tried to outsmart GCC by manual review :/ IOW my R-b stands: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> >> if nfds == 0, we don't enter in the loop and ret is never set. >> >> Thanks, >> Laurent >> >
On 2020/7/2 上午3:39, Laurent Vivier wrote: > +void qemu_set_nonblock(int fd) > +{ > + int f; > + f = qemu_try_set_nonblock(fd); > + assert(f == 0); > } So we keep this assert which means it can still be triggered from monitor? I don't check all the callers, but I got: in tap_init_one() we had: if (vhostfdname) { vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err); if (vhostfd == -1) { if (tap->has_vhostforce && tap->vhostforce) { error_propagate(errp, err); } else { warn_report_err(err); } return; } qemu_set_nonblock(vhostfd); } else { and in net_init_socket() we had: if (sock->has_fd) { int fd; fd = monitor_fd_param(cur_mon, sock->fd, errp); if (fd == -1) { return -1; } qemu_set_nonblock(fd); if (!net_socket_fd_init(peer, "socket", name, fd, 1, sock->mcast, errp)) { return -1; } return 0; } Thanks
On 07/07/2020 09:56, Jason Wang wrote: > > On 2020/7/2 上午3:39, Laurent Vivier wrote: >> +void qemu_set_nonblock(int fd) >> +{ >> + int f; >> + f = qemu_try_set_nonblock(fd); >> + assert(f == 0); >> } > > > So we keep this assert which means it can still be triggered from monitor? > > I don't check all the callers, but I got: > > in tap_init_one() we had: > > if (vhostfdname) { > vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err); > if (vhostfd == -1) { > if (tap->has_vhostforce && tap->vhostforce) { > error_propagate(errp, err); > } else { > warn_report_err(err); > } > return; > } > qemu_set_nonblock(vhostfd); > } else { > > and in net_init_socket() we had: > > if (sock->has_fd) { > int fd; > > fd = monitor_fd_param(cur_mon, sock->fd, errp); > if (fd == -1) { > return -1; > } > qemu_set_nonblock(fd); > if (!net_socket_fd_init(peer, "socket", name, fd, 1, sock->mcast, > errp)) { > return -1; > } > return 0; > } > Yes, I think you're right, all fd that come from monitor and passed to qemu_set_nonblock() before any other use should use qemu_try_set_nonblock() and report an error. I update my patch accordingly. Thanks, Laurent
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h index 57cd049d6edd..7d1f8135767d 100644 --- a/include/qemu/sockets.h +++ b/include/qemu/sockets.h @@ -18,6 +18,7 @@ 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); void qemu_set_block(int fd); +int qemu_try_set_nonblock(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..fb04c9044ce2 100644 --- a/net/tap.c +++ b/net/tap.c @@ -766,6 +766,7 @@ int net_init_tap(const Netdev *netdev, const char *name, Error *err = NULL; const char *vhostfdname; char ifname[128]; + int ret = 0; assert(netdev->type == NET_CLIENT_DRIVER_TAP); tap = &netdev->u.tap; @@ -795,7 +796,12 @@ int net_init_tap(const Netdev *netdev, const char *name, return -1; } - qemu_set_nonblock(fd); + ret = qemu_try_set_nonblock(fd); + if (ret < 0) { + error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d", + name, fd); + return -1; + } vnet_hdr = tap_probe_vnet_hdr(fd); @@ -810,7 +816,6 @@ int net_init_tap(const Netdev *netdev, const char *name, char **fds; char **vhost_fds; int nfds = 0, nvhosts = 0; - int ret = 0; if (tap->has_ifname || tap->has_script || tap->has_downscript || tap->has_vnet_hdr || tap->has_helper || tap->has_queues || @@ -843,7 +848,12 @@ int net_init_tap(const Netdev *netdev, const char *name, goto free_fail; } - qemu_set_nonblock(fd); + ret = qemu_try_set_nonblock(fd); + if (ret < 0) { + error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d", + name, fd); + goto free_fail; + } if (i == 0) { vnet_hdr = tap_probe_vnet_hdr(fd); diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 916f1be2243a..149254cd691f 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -253,25 +253,35 @@ void qemu_set_block(int fd) assert(f != -1); } -void qemu_set_nonblock(int fd) +int qemu_try_set_nonblock(int fd) { int f; f = fcntl(fd, F_GETFL); - assert(f != -1); - f = fcntl(fd, F_SETFL, f | O_NONBLOCK); -#ifdef __OpenBSD__ if (f == -1) { + return -errno; + } + if (fcntl(fd, F_SETFL, f | O_NONBLOCK) == -1) { +#ifdef __OpenBSD__ /* * Previous to OpenBSD 6.3, fcntl(F_SETFL) is not permitted on * memory devices and sets errno to ENODEV. * It's OK if we fail to set O_NONBLOCK on devices like /dev/null, * because they will never block anyway. */ - assert(errno == ENODEV); - } -#else - assert(f != -1); + if (errno == ENODEV) { + return 0; + } #endif + return -errno; + } + return 0; +} + +void qemu_set_nonblock(int fd) +{ + int f; + f = qemu_try_set_nonblock(fd); + assert(f == 0); } int socket_set_fast_reuse(int fd) diff --git a/util/oslib-win32.c b/util/oslib-win32.c index e9b14ab17847..5548ce6038f3 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -132,31 +132,6 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) } #endif /* CONFIG_LOCALTIME_R */ -void qemu_set_block(int fd) -{ - unsigned long opt = 0; - WSAEventSelect(fd, NULL, 0); - ioctlsocket(fd, FIONBIO, &opt); -} - -void qemu_set_nonblock(int fd) -{ - unsigned long opt = 1; - ioctlsocket(fd, FIONBIO, &opt); - qemu_fd_register(fd); -} - -int socket_set_fast_reuse(int fd) -{ - /* Enabling the reuse of an endpoint that was used by a socket still in - * TIME_WAIT state is usually performed by setting SO_REUSEADDR. On Windows - * fast reuse is the default and SO_REUSEADDR does strange things. So we - * don't have to do anything here. More info can be found at: - * http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx */ - return 0; -} - - static int socket_error(void) { switch (WSAGetLastError()) { @@ -233,6 +208,38 @@ static int socket_error(void) } } +void qemu_set_block(int fd) +{ + unsigned long opt = 0; + WSAEventSelect(fd, NULL, 0); + ioctlsocket(fd, FIONBIO, &opt); +} + +int qemu_try_set_nonblock(int fd) +{ + unsigned long opt = 1; + if (ioctlsocket(fd, FIONBIO, &opt) != NO_ERROR) { + return -socket_error(); + } + qemu_fd_register(fd); + return 0; +} + +void qemu_set_nonblock(int fd) +{ + (void)qemu_try_set_nonblock(fd); +} + +int socket_set_fast_reuse(int fd) +{ + /* Enabling the reuse of an endpoint that was used by a socket still in + * TIME_WAIT state is usually performed by setting SO_REUSEADDR. On Windows + * fast reuse is the default and SO_REUSEADDR does strange things. So we + * don't have to do anything here. More info can be found at: + * http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx */ + return 0; +} + int inet_aton(const char *cp, struct in_addr *ia) { uint32_t addr = inet_addr(cp);
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, add a function, qemu_try_set_nonblock(), that allows to report the problem without crashing. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- include/qemu/sockets.h | 1 + net/tap.c | 16 +++++++++--- util/oslib-posix.c | 26 +++++++++++++------ util/oslib-win32.c | 57 ++++++++++++++++++++++++------------------ 4 files changed, 64 insertions(+), 36 deletions(-)