Re: [Qemu-devel] [PATCH 1/6] qemu-bridge-helper: Fix misuse of isspace()

[Daniel P . Berrangé]

On Wed, May 15, 2019 at 08:34:17AM +0200, Markus Armbruster wrote:
> Jason Wang <address@hidden> writes:
> 
> > On 2019/5/14 下午8:18, Markus Armbruster wrote:
> >> Peter Maydell <address@hidden> writes:
> >>
> >>> On Mon, 13 May 2019 at 14:21, Markus Armbruster <address@hidden> wrote:
> >>>> Perhaps I should do it just for this file while I touch it anyway.  The
> >>>> question to ask: should parse_acl_file() obey the locale for whitespace
> >>>> recognition?
> >>> I vote for "no".
> >>>
> >>> Q: do we document the format of the ACL file anywhere ?
> >> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
> >> documentation.
> >>
> >> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
> >> -help output and some .texi that goes into qemu-doc and the manual page.
> >> None of it mentions how qemu-bridge-helper is run, or the ACL file
> >> feature, let alone what its format might be.
> >>
> >> I'm afraid all we have is the commit message.  Which doesn't really
> >> define the file format, it merely gives a bunch of examples.
> >>
> >> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
> >> -netdev bridge.
> >>
> >> Both variations of -netdev call net_bridge_run_helper() to run the
> >> helper.  First argument is -netdev parameter "helper", default usually
> >> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
> >> "br", default "br0".
> >>
> >> If @helper contains space or tab, net_bridge_run_helper() guesses its a
> >> full command, else it guesses its the name of the executable.  Bad
> >> magic.
> >>
> >> If it guesses name of executable, it execv()s this executable with
> >> arguments "--use-vnet", "--fd=FD", "address@hidden".
> >>
> >> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
> >> the helper's half of the socketpair used to connect QEMU and the helper.
> >> It further appends "address@hidden", unless @helper contains "--br=".
> >> More bad magic.
> >>
> >> It executes the resulting string with sh -c.  Magic cherry on top.
> >>
> >> When the helper fails, netdev creation fails.
> >>
> >> The helper we ship with QEMU unconditionally tries to read
> >> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
> >> Errors in this file are fatal.  Errors in files it includes are not
> >> fatal; instead, the remainder of the erroneous file is ignored.
> >> *Boggle*
> >>
> >> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
> >> commit 2d80fbb14df).  Makes sense, because running QEMU with the
> >> necessary privileges would be unwise, and so would be letting it execute
> >> setuid helpers.  Also bypasses the bad magic in QEMU's
> >> net_bridge_run_helper().
> >
> >
> > I don't notice this before. Is this only for the convenience of
> > development? I guess libvirt should have native support like adding
> > port to bridge/OVS without the help any external command or script.
> 
> Commit 2d80fbb14df hints at the reason:
> 
>     <source type='bridge'> uses a helper application to do the necessary
>     TUN/TAP setup to use an existing network bridge, thus letting
>     unprivileged users use TUN/TAP interfaces.
>     ~~~~~~~~~~~~~~~~~~
> 
> The code confirms:
> 
>     /* qemuInterfaceBridgeConnect:
>      * @def: the definition of the VM
>      * @driver: qemu driver data
>      * @net: pointer to the VM's interface description
>      * @tapfd: array of file descriptor return value for the new device
>      * @tapfdsize: number of file descriptors in @tapfd
>      *
> ---> * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_NETWORK or
> ---> * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap
>      * device connecting to a bridge device)
>      */
>     int
>     qemuInterfaceBridgeConnect(virDomainDefPtr def,
>                                virQEMUDriverPtr driver,
>                                virDomainNetDefPtr net,
>                                int *tapfd,
>                                size_t *tapfdSize)
>     {
>         [...]
> --->    if (virQEMUDriverIsPrivileged(driver)) {
>             [...]
>         } else {
>             if (qemuCreateInBridgePortWithHelper(cfg, brname,
>                                                  &net->ifname,
>                                                  tapfd, tap_create_flags) < 
> 0) {
>                 virDomainAuditNetDevice(def, net, tunpath, false);
>                 goto cleanup;
>             }
>             [...]
>         }
>         [...]
>     }
> 
> >> qemu-bridge-helper should have a manual page, and its handling of errors
> >> in ACL include files needs work.  There's probably more; I just glanced
> >> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
> >> add it to Jason's "Network device backends"?
> >
> >
> > Yes.
> >
> >> -netdev's helper parameter is seriously underdocumented.  Document or
> >> deprecate?
> >
> >
> > I believe management should only use fd parameter of TAP. If we have
> > other, it should be a duplication. So I suggest to deprecate the
> > bridge helper and -netdev bridge.
> 
> Objections, anyone?

Libvirt runs the qemu bridge helper command directly, and we have
applications using this functionality.

I'd like libvirt to be able to avoid use of the QEMU bridge helper and
instead have unprivileged libvirt talk to privileged libvirtd to open a
TAP fd on its behalf. If we ever get that done, then libvirt would not
need the qemu bridge helper command anymore.

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