Re: [Qemu-devel] [PATCH] net: detect errors from probing vnet hdr flag for TAP devices

["Daniel P. Berrange" <berrange@redhat.com>]

On Mon, Oct 30, 2017 at 03:37:33PM +0800, Jason Wang wrote:
> 
> 
> On 2017年10月27日 16:55, Daniel P. Berrange wrote:
> > 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>
> > ---
> >   net/tap-bsd.c     |  2 +-
> >   net/tap-linux.c   | 12 +++++++++---
> >   net/tap-solaris.c |  2 +-
> >   net/tap-stub.c    |  2 +-
> >   net/tap.c         | 25 ++++++++++++++++++++-----
> >   net/tap_int.h     |  2 +-
> >   6 files changed, 33 insertions(+), 12 deletions(-)
> > 
> > diff --git a/net/tap-bsd.c b/net/tap-bsd.c
> > index 6c9692263d..4f1d633b08 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 535b1ddb61..de74928407 100644
> > --- a/net/tap-linux.c
> > +++ b/net/tap-linux.c
> > @@ -147,13 +147,19 @@ 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;
> > +        /* Kernel pre-dates TUNGETIFF support */
> > +        if (errno == -EINVAL) {
> > +            return 0;
> 
> This looks still unsafe, e.g some other device may return -EINVAL too.
> 
> Is this better to check stat.st_rdev through fstat()?

Hmm, yes, that might be possible. Let me investigate...


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|