[PATCH v3 0/2] net: tap: check file descriptor can be used

[PATCH v3 0/2] net: tap: check file descriptor can be used

[Laurent Vivier]

v3: move qemu_fd_is_valid() checking into a new function
    qemu_try_set_nonblock(), and use qemu_try_set_nonblock() in
    qemu_set_nonblock().

v2: Add patch from Daniel to check the fd can be used

    I have updated Daniel's patch not to check for EINVAL on TUNGETIFF
    as I think we can avoid this special case because TUNGETIFF
    is available since kernel v2.6.27 (October 2008)
    Moreover I think the code was wrong as it was checking with -EINVAL and
    not EINVAL.

Daniel P. Berrang�� (1):
  net: detect errors from probing vnet hdr flag for TAP devices

Laurent Vivier (1):
  net: tap: check if the file descriptor is valid before using it

 include/qemu/sockets.h |  1 +
 net/tap-bsd.c          |  2 +-
 net/tap-linux.c        |  8 +++---
 net/tap-solaris.c      |  2 +-
 net/tap-stub.c         |  2 +-
 net/tap.c              | 41 ++++++++++++++++++++++++------
 net/tap_int.h          |  2 +-
 util/oslib-posix.c     | 26 +++++++++++++------
 util/oslib-win32.c     | 57 ++++++++++++++++++++++++------------------
 9 files changed, 93 insertions(+), 48 deletions(-)

-- 
2.26.2

[PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it

[Laurent Vivier]

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;
 
     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);
-- 
2.26.2

Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it

[Philippe Mathieu-Daudé]

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);
> 

Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it

[Laurent Vivier]

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

Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it

[Philippe Mathieu-Daudé]

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
> 

Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it

[Philippe Mathieu-Daudé]

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
>>
> 

Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it

[Jason Wang]

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

Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it

[Laurent Vivier]

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

[PATCH v3 2/2] net: detect errors from probing vnet hdr flag for TAP devices

[Laurent Vivier]

From: "Daniel P. Berrange" <berrange@redhat.com>

When QEMU sets up a tap based network device backend, it mostly ignores errors
reported from various ioctl() calls it makes, assuming the TAP file descriptor
is valid. This assumption can easily be violated when the user is passing in a
pre-opened file descriptor. At best, the ioctls may fail with a -EBADF, but if
the user passes in a bogus FD number that happens to clash with a FD number that
QEMU has opened internally for another reason, a wide variety of errnos may
result, as the TUNGETIFF ioctl number may map to a completely different command
on a different type of file.

By ignoring all these errors, QEMU sets up a zombie network backend that will
never pass any data. Even worse, when QEMU shuts down, or that network backend
is hot-removed, it will close this bogus file descriptor, which could belong to
another QEMU device backend.

There's no obvious guaranteed reliable way to detect that a FD genuinely is a
TAP device, as opposed to a UNIX socket, or pipe, or something else. Checking
the errno from probing vnet hdr flag though, does catch the big common cases.
ie calling TUNGETIFF will return EBADF for an invalid FD, and ENOTTY when FD is
a UNIX socket, or pipe which catches accidental collisions with FDs used for
stdio, or monitor socket.

Previously the example below where bogus fd 9 collides with the FD used for the
chardev saw:

$ ./x86_64-softmmu/qemu-system-x86_64 -netdev tap,id=hostnet0,fd=9 \
  -chardev socket,id=charchannel0,path=/tmp/qga,server,nowait \
  -monitor stdio -vnc :0
qemu-system-x86_64: -netdev tap,id=hostnet0,fd=9: TUNGETIFF ioctl() failed: Inappropriate ioctl for device
TUNSETOFFLOAD ioctl() failed: Bad address
QEMU 2.9.1 monitor - type 'help' for more information
(qemu) Warning: netdev hostnet0 has no peer

which gives a running QEMU with a zombie network backend.

With this change applied we get an error message and QEMU immediately exits
before carrying on and making a bigger disaster:

$ ./x86_64-softmmu/qemu-system-x86_64 -netdev tap,id=hostnet0,fd=9 \
  -chardev socket,id=charchannel0,path=/tmp/qga,server,nowait \
  -monitor stdio -vnc :0
qemu-system-x86_64: -netdev tap,id=hostnet0,vhost=on,fd=9: Unable to query TUNGETIFF on FD 9: Inappropriate ioctl for device

Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-id: 20171027085548.3472-1-berrange@redhat.com
[lv: to simplify, don't check on EINVAL with TUNGETIFF as it exists since v2.6.27]
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 net/tap-bsd.c     |  2 +-
 net/tap-linux.c   |  8 +++++---
 net/tap-solaris.c |  2 +-
 net/tap-stub.c    |  2 +-
 net/tap.c         | 25 ++++++++++++++++++++-----
 net/tap_int.h     |  2 +-
 6 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index a5c3707f806d..77aaf674b19d 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -211,7 +211,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
 }
 
-int tap_probe_vnet_hdr(int fd)
+int tap_probe_vnet_hdr(int fd, Error **errp)
 {
     return 0;
 }
diff --git a/net/tap-linux.c b/net/tap-linux.c
index e0dd442ee34f..b0635e9e32ce 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -147,13 +147,15 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
     }
 }
 
-int tap_probe_vnet_hdr(int fd)
+int tap_probe_vnet_hdr(int fd, Error **errp)
 {
     struct ifreq ifr;
 
     if (ioctl(fd, TUNGETIFF, &ifr) != 0) {
-        error_report("TUNGETIFF ioctl() failed: %s", strerror(errno));
-        return 0;
+        /* TUNGETIFF is available since kernel v2.6.27 */
+        error_setg_errno(errp, errno,
+                         "Unable to query TUNGETIFF on FD %d", fd);
+        return -1;
     }
 
     return ifr.ifr_flags & IFF_VNET_HDR;
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index 4725d2314eef..ae2ba6828415 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -206,7 +206,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
 }
 
-int tap_probe_vnet_hdr(int fd)
+int tap_probe_vnet_hdr(int fd, Error **errp)
 {
     return 0;
 }
diff --git a/net/tap-stub.c b/net/tap-stub.c
index a9ab8f829362..de525a2e69d4 100644
--- a/net/tap-stub.c
+++ b/net/tap-stub.c
@@ -37,7 +37,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
 }
 
-int tap_probe_vnet_hdr(int fd)
+int tap_probe_vnet_hdr(int fd, Error **errp)
 {
     return 0;
 }
diff --git a/net/tap.c b/net/tap.c
index fb04c9044ce2..208d9c0f8d35 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -597,7 +597,11 @@ int net_init_bridge(const Netdev *netdev, const char *name,
     }
 
     qemu_set_nonblock(fd);
-    vnet_hdr = tap_probe_vnet_hdr(fd);
+    vnet_hdr = tap_probe_vnet_hdr(fd, errp);
+    if (vnet_hdr < 0) {
+        close(fd);
+        return -1;
+    }
     s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr);
 
     snprintf(s->nc.info_str, sizeof(s->nc.info_str), "helper=%s,br=%s", helper,
@@ -803,7 +807,11 @@ int net_init_tap(const Netdev *netdev, const char *name,
             return -1;
         }
 
-        vnet_hdr = tap_probe_vnet_hdr(fd);
+        vnet_hdr = tap_probe_vnet_hdr(fd, errp);
+        if (vnet_hdr < 0) {
+            close(fd);
+            return -1;
+        }
 
         net_init_tap_one(tap, peer, "tap", name, NULL,
                          script, downscript,
@@ -856,8 +864,11 @@ int net_init_tap(const Netdev *netdev, const char *name,
             }
 
             if (i == 0) {
-                vnet_hdr = tap_probe_vnet_hdr(fd);
-            } else if (vnet_hdr != tap_probe_vnet_hdr(fd)) {
+                vnet_hdr = tap_probe_vnet_hdr(fd, errp);
+                if (vnet_hdr < 0) {
+                    goto free_fail;
+                }
+            } else if (vnet_hdr != tap_probe_vnet_hdr(fd, NULL)) {
                 error_setg(errp,
                            "vnet_hdr not consistent across given tap fds");
                 ret = -1;
@@ -902,7 +913,11 @@ free_fail:
         }
 
         qemu_set_nonblock(fd);
-        vnet_hdr = tap_probe_vnet_hdr(fd);
+        vnet_hdr = tap_probe_vnet_hdr(fd, errp);
+        if (vnet_hdr < 0) {
+            close(fd);
+            return -1;
+        }
 
         net_init_tap_one(tap, peer, "bridge", name, ifname,
                          script, downscript, vhostfdname,
diff --git a/net/tap_int.h b/net/tap_int.h
index e3194b23f47d..225a49ea4843 100644
--- a/net/tap_int.h
+++ b/net/tap_int.h
@@ -34,7 +34,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
 ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
 
 void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
-int tap_probe_vnet_hdr(int fd);
+int tap_probe_vnet_hdr(int fd, Error **errp);
 int tap_probe_vnet_hdr_len(int fd, int len);
 int tap_probe_has_ufo(int fd);
 void tap_fd_set_offload(int fd, int csum, int tso4, int tso6, int ecn, int ufo);
-- 
2.26.2

Re: [PATCH v3 2/2] net: detect errors from probing vnet hdr flag for TAP devices

[Philippe Mathieu-Daudé]

On 7/1/20 9:39 PM, Laurent Vivier wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> When QEMU sets up a tap based network device backend, it mostly ignores errors
> reported from various ioctl() calls it makes, assuming the TAP file descriptor
> is valid. This assumption can easily be violated when the user is passing in a
> pre-opened file descriptor. At best, the ioctls may fail with a -EBADF, but if
> the user passes in a bogus FD number that happens to clash with a FD number that
> QEMU has opened internally for another reason, a wide variety of errnos may
> result, as the TUNGETIFF ioctl number may map to a completely different command
> on a different type of file.
> 
> By ignoring all these errors, QEMU sets up a zombie network backend that will
> never pass any data. Even worse, when QEMU shuts down, or that network backend
> is hot-removed, it will close this bogus file descriptor, which could belong to
> another QEMU device backend.
> 
> There's no obvious guaranteed reliable way to detect that a FD genuinely is a
> TAP device, as opposed to a UNIX socket, or pipe, or something else. Checking
> the errno from probing vnet hdr flag though, does catch the big common cases.
> ie calling TUNGETIFF will return EBADF for an invalid FD, and ENOTTY when FD is
> a UNIX socket, or pipe which catches accidental collisions with FDs used for
> stdio, or monitor socket.
> 
> Previously the example below where bogus fd 9 collides with the FD used for the
> chardev saw:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -netdev tap,id=hostnet0,fd=9 \
>   -chardev socket,id=charchannel0,path=/tmp/qga,server,nowait \
>   -monitor stdio -vnc :0
> qemu-system-x86_64: -netdev tap,id=hostnet0,fd=9: TUNGETIFF ioctl() failed: Inappropriate ioctl for device
> TUNSETOFFLOAD ioctl() failed: Bad address
> QEMU 2.9.1 monitor - type 'help' for more information
> (qemu) Warning: netdev hostnet0 has no peer
> 
> which gives a running QEMU with a zombie network backend.
> 
> With this change applied we get an error message and QEMU immediately exits
> before carrying on and making a bigger disaster:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -netdev tap,id=hostnet0,fd=9 \
>   -chardev socket,id=charchannel0,path=/tmp/qga,server,nowait \
>   -monitor stdio -vnc :0
> qemu-system-x86_64: -netdev tap,id=hostnet0,vhost=on,fd=9: Unable to query TUNGETIFF on FD 9: Inappropriate ioctl for device
> 
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Message-id: 20171027085548.3472-1-berrange@redhat.com
> [lv: to simplify, don't check on EINVAL with TUNGETIFF as it exists since v2.6.27]
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  net/tap-bsd.c     |  2 +-
>  net/tap-linux.c   |  8 +++++---
>  net/tap-solaris.c |  2 +-
>  net/tap-stub.c    |  2 +-
>  net/tap.c         | 25 ++++++++++++++++++++-----
>  net/tap_int.h     |  2 +-
>  6 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/net/tap-bsd.c b/net/tap-bsd.c
> index a5c3707f806d..77aaf674b19d 100644
> --- a/net/tap-bsd.c
> +++ b/net/tap-bsd.c
> @@ -211,7 +211,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>  {
>  }
>  
> -int tap_probe_vnet_hdr(int fd)
> +int tap_probe_vnet_hdr(int fd, Error **errp)
>  {
>      return 0;
>  }
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index e0dd442ee34f..b0635e9e32ce 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -147,13 +147,15 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>      }
>  }
>  
> -int tap_probe_vnet_hdr(int fd)
> +int tap_probe_vnet_hdr(int fd, Error **errp)
>  {
>      struct ifreq ifr;
>  
>      if (ioctl(fd, TUNGETIFF, &ifr) != 0) {
> -        error_report("TUNGETIFF ioctl() failed: %s", strerror(errno));
> -        return 0;
> +        /* TUNGETIFF is available since kernel v2.6.27 */
> +        error_setg_errno(errp, errno,
> +                         "Unable to query TUNGETIFF on FD %d", fd);
> +        return -1;
>      }
>  
>      return ifr.ifr_flags & IFF_VNET_HDR;
> diff --git a/net/tap-solaris.c b/net/tap-solaris.c
> index 4725d2314eef..ae2ba6828415 100644
> --- a/net/tap-solaris.c
> +++ b/net/tap-solaris.c
> @@ -206,7 +206,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>  {
>  }
>  
> -int tap_probe_vnet_hdr(int fd)
> +int tap_probe_vnet_hdr(int fd, Error **errp)
>  {
>      return 0;
>  }
> diff --git a/net/tap-stub.c b/net/tap-stub.c
> index a9ab8f829362..de525a2e69d4 100644
> --- a/net/tap-stub.c
> +++ b/net/tap-stub.c
> @@ -37,7 +37,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>  {
>  }
>  
> -int tap_probe_vnet_hdr(int fd)
> +int tap_probe_vnet_hdr(int fd, Error **errp)
>  {
>      return 0;
>  }
> diff --git a/net/tap.c b/net/tap.c
> index fb04c9044ce2..208d9c0f8d35 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -597,7 +597,11 @@ int net_init_bridge(const Netdev *netdev, const char *name,
>      }
>  
>      qemu_set_nonblock(fd);
> -    vnet_hdr = tap_probe_vnet_hdr(fd);
> +    vnet_hdr = tap_probe_vnet_hdr(fd, errp);
> +    if (vnet_hdr < 0) {
> +        close(fd);
> +        return -1;
> +    }
>      s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr);
>  
>      snprintf(s->nc.info_str, sizeof(s->nc.info_str), "helper=%s,br=%s", helper,
> @@ -803,7 +807,11 @@ int net_init_tap(const Netdev *netdev, const char *name,
>              return -1;
>          }
>  
> -        vnet_hdr = tap_probe_vnet_hdr(fd);
> +        vnet_hdr = tap_probe_vnet_hdr(fd, errp);
> +        if (vnet_hdr < 0) {
> +            close(fd);
> +            return -1;
> +        }
>  
>          net_init_tap_one(tap, peer, "tap", name, NULL,
>                           script, downscript,
> @@ -856,8 +864,11 @@ int net_init_tap(const Netdev *netdev, const char *name,
>              }
>  
>              if (i == 0) {
> -                vnet_hdr = tap_probe_vnet_hdr(fd);
> -            } else if (vnet_hdr != tap_probe_vnet_hdr(fd)) {
> +                vnet_hdr = tap_probe_vnet_hdr(fd, errp);
> +                if (vnet_hdr < 0) {
> +                    goto free_fail;
> +                }
> +            } else if (vnet_hdr != tap_probe_vnet_hdr(fd, NULL)) {
>                  error_setg(errp,
>                             "vnet_hdr not consistent across given tap fds");
>                  ret = -1;
> @@ -902,7 +913,11 @@ free_fail:
>          }
>  
>          qemu_set_nonblock(fd);
> -        vnet_hdr = tap_probe_vnet_hdr(fd);
> +        vnet_hdr = tap_probe_vnet_hdr(fd, errp);
> +        if (vnet_hdr < 0) {
> +            close(fd);
> +            return -1;
> +        }
>  
>          net_init_tap_one(tap, peer, "bridge", name, ifname,
>                           script, downscript, vhostfdname,
> diff --git a/net/tap_int.h b/net/tap_int.h
> index e3194b23f47d..225a49ea4843 100644
> --- a/net/tap_int.h
> +++ b/net/tap_int.h
> @@ -34,7 +34,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
>  
>  void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
> -int tap_probe_vnet_hdr(int fd);
> +int tap_probe_vnet_hdr(int fd, Error **errp);

Missing documentation (can be added on top), otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  int tap_probe_vnet_hdr_len(int fd, int len);
>  int tap_probe_has_ufo(int fd);
>  void tap_fd_set_offload(int fd, int csum, int tso4, int tso6, int ecn, int ufo);
> 

Re: [PATCH v3 0/2] net: tap: check file descriptor can be used

[Laurent Vivier]

Hi,

ping.

I think it would be good to have this fix in next release.

Thanks,
Laurent

On 01/07/2020 21:39, Laurent Vivier wrote:
> v3: move qemu_fd_is_valid() checking into a new function
>     qemu_try_set_nonblock(), and use qemu_try_set_nonblock() in
>     qemu_set_nonblock().
> 
> v2: Add patch from Daniel to check the fd can be used
> 
>     I have updated Daniel's patch not to check for EINVAL on TUNGETIFF
>     as I think we can avoid this special case because TUNGETIFF
>     is available since kernel v2.6.27 (October 2008)
>     Moreover I think the code was wrong as it was checking with -EINVAL and
>     not EINVAL.
> 
> Daniel P. Berrang�� (1):
>   net: detect errors from probing vnet hdr flag for TAP devices
> 
> Laurent Vivier (1):
>   net: tap: check if the file descriptor is valid before using it
> 
>  include/qemu/sockets.h |  1 +
>  net/tap-bsd.c          |  2 +-
>  net/tap-linux.c        |  8 +++---
>  net/tap-solaris.c      |  2 +-
>  net/tap-stub.c         |  2 +-
>  net/tap.c              | 41 ++++++++++++++++++++++++------
>  net/tap_int.h          |  2 +-
>  util/oslib-posix.c     | 26 +++++++++++++------
>  util/oslib-win32.c     | 57 ++++++++++++++++++++++++------------------
>  9 files changed, 93 insertions(+), 48 deletions(-)
>