On 7/1/20 9:39 PM, Laurent Vivier wrote:
> From: "Daniel P. Berrange" <berra...@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 <dgilb...@redhat.com>
> Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> Tested-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> Message-id: 20171027085548.3472-1-berra...@redhat.com
> [lv: to simplify, don't check on EINVAL with TUNGETIFF as it exists since
> v2.6.27]
> Signed-off-by: Laurent Vivier <lviv...@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é <phi...@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);
>
