| Message ID | 20200701193951.36248-3-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: > 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); >
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);