Re: [PATCH v3 01/36] network: restrict usage of port management APIs

[Laine Stump <laine@xxxxxxxxx>]

On 3/21/19 8:27 PM, Cole Robinson wrote:
> Okay so I needed to do some studying to understand what's going on in
> the first part of this series. Just gonna type some notes here:
>
> virDomainActualNetDef tracks all the data we need to convert a
> virNetworkPtr content to a virDomainNetDef <source>. It's only ever
> filled in for <interface type='network'> when a domain is running.
> hypervisor drivers fill the data in at VM startup by calling
> virDomainNetAllocateActualDevice, which hands the DomainNetDefPtr off to
> bridge_drive.c callbacks, which serialize the net config into
> virDomainActualNetDef.

More or less correct, but I might word a couple things slightly 
differently. A virDomainActualNetDef describes the network device that 
was constructed at runtime out of the combination of an <interface 
type='network'> (virDomainNetDef) and the network referenced by that 
interface (virNetwork). Construction of the virDomainActualNetDef is 
handled by virDomainNetAllocateActualDevice (aka 
networkAllocateActualDevice()). (Another way of thinking about it is 
that the virDomainNetDef and virNetwork are the config, and 
virDomainActualNetDef is the status).


> One desired end goal of this series is making sure that bridge_driver is
> not messing with any Domain*Def stuff. The first 10-15 patches here are
> unwinding some pieces of that before it gets into the real new stuff.
>
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > The port allocation APIs are currently called unconditionally for all
> > types of NIC, but (mostly) only do anything for NICs with type=network.
> >
> > The exception is the port allocate API which does some validation even
> > for NICs with type!=network. Relying on this validation is flawed,
> > however, since the network driver may not even be installed. IOW virt
> > drivers must not delegate validation to the network driver for NICs
> > with type != network.


Yes, the presence of those bits was due to my mistaken/misguided idea 
that all network device setup could/should eventually be done by the 
network driver (in order to enable unprivileged libvirt hypervisor 
drivers to connect to the privileged libvirt network driver for all 
their networking needs). I failed to think about the possible need for 
libvirtd to run without any network driver, and that scenario makes my 
wish (partly) a bust (it may be useful for the network driver to *be 
able* to perform all operations related to setting up a network device 
that require elevated privileges, but it can't be *required*).


> > This change allows us to report errors when the virtual network driver
> > is not registered.
> For this and the following patch I don't really follow why we can't put
> the iface->type == TYPE_NETWORK and the netconn = virGetConnectNetwork()
> directly into the domain_conf.c bridge_driver callback wrappers like
> virDomainNetAllocateActualDevice. After the final patch in this series,
> the open coded netconn lookup and TYPE_NETWORK checks are still there,
> and the function signature hasn't changed. Am I missing something?
>
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> >   src/conf/domain_conf.c      | 26 ++++++++------
> >   src/libxl/libxl_domain.c    |  6 ++--
> >   src/libxl/libxl_driver.c    |  9 +++--
> >   src/lxc/lxc_driver.c        |  6 ++--
> >   src/lxc/lxc_process.c       |  9 +++--
> >   src/network/bridge_driver.c | 72 +++++++++++++++++++------------------
> >   src/qemu/qemu_driver.c      |  6 ++--
> >   src/qemu/qemu_hotplug.c     | 17 +++++----
> >   src/qemu/qemu_process.c     |  9 +++--
> >   9 files changed, 94 insertions(+), 66 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 504c24b545..b7bb94b90b 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -30187,13 +30187,11 @@ int
> >   virDomainNetAllocateActualDevice(virDomainDefPtr dom,
> >                                    virDomainNetDefPtr iface)
> >   {
> > -    /* Just silently ignore if network driver isn't present. If something
> > -     * has tried to use a NIC with type=network, other code will already
> > -     * cause an error. This ensures type=bridge doesn't break when
> > -     * network driver is compiled out.
> > -     */
> > -    if (!netAllocate)
> > -        return 0;
> > +    if (!netAllocate) {
> > +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> > +                       _("Virtual networking driver is not available"));
> > +        return -1;
> > +    }
> >   
> >       return netAllocate(dom, iface);
> >   }
> > @@ -30223,8 +30221,11 @@ bool
> >   virDomainNetBandwidthChangeAllowed(virDomainNetDefPtr iface,
> >                                      virNetDevBandwidthPtr newBandwidth)
> >   {
> > -    if (!netBandwidthChangeAllowed)
> > -        return 0;
> > +    if (!netBandwidthChangeAllowed) {
> > +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> > +                       _("Virtual networking driver is not available"));
> > +        return -1;
> > +    }
> >   
> >       return netBandwidthChangeAllowed(iface, newBandwidth);
> >   }
> > @@ -30233,8 +30234,11 @@ int
> >   virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,
> >                               virNetDevBandwidthPtr newBandwidth)
> >   {
> > -    if (!netBandwidthUpdate)
> > -        return 0;
> > +    if (!netBandwidthUpdate) {
> > +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> > +                       _("Virtual networking driver is not available"));
> > +        return -1;
> > +    }
> >   
> >       return netBandwidthUpdate(iface, newBandwidth);
> >   }
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > index 287406d323..e2f1b54210 100644
> > --- a/src/libxl/libxl_domain.c
> > +++ b/src/libxl/libxl_domain.c
> > @@ -903,7 +903,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
> >   
> >               /* cleanup actual device */
> >               virDomainNetRemoveHostdev(vm->def, net);
> > -            virDomainNetReleaseActualDevice(vm->def, net);
> > +            if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> > +                virDomainNetReleaseActualDevice(vm->def, net);
> >           }
> >       }
> >   
> > @@ -1060,7 +1061,8 @@ libxlNetworkPrepareDevices(virDomainDefPtr def)
> >            * network's pool of devices, or resolve bridge device name
> >            * to the one defined in the network definition.
> >            */
> > -        if (virDomainNetAllocateActualDevice(def, net) < 0)
> > +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > +            virDomainNetAllocateActualDevice(def, net) < 0)
> >               return -1;
> >   
> >           actualType = virDomainNetGetActualType(net);
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > index a9edc8211d..d5cd3fc834 100644
> > --- a/src/libxl/libxl_driver.c
> > +++ b/src/libxl/libxl_driver.c
> > @@ -3390,7 +3390,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
> >        * network's pool of devices, or resolve bridge device name
> >        * to the one defined in the network definition.
> >        */
> > -    if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
> > +    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > +        virDomainNetAllocateActualDevice(vm->def, net) < 0)
> >           goto cleanup;
> >   
> >       actualType = virDomainNetGetActualType(net);
> > @@ -3440,7 +3441,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
> >           vm->def->nets[vm->def->nnets++] = net;
> >       } else {
> >           virDomainNetRemoveHostdev(vm->def, net);
> > -        virDomainNetReleaseActualDevice(vm->def, net);
> > +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> > +            virDomainNetReleaseActualDevice(vm->def, net);
> >       }
> >       virObjectUnref(cfg);
> >       return ret;
> > @@ -3863,7 +3865,8 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,
> >    cleanup:
> >       libxl_device_nic_dispose(&nic);
> >       if (!ret) {
> > -        virDomainNetReleaseActualDevice(vm->def, detach);
> > +        if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> > +            virDomainNetReleaseActualDevice(vm->def, detach);
> >           virDomainNetRemove(vm->def, detachidx);
> >       }
> >       virObjectUnref(cfg);
> > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> > index e981f8e901..027c4fd990 100644
> > --- a/src/lxc/lxc_driver.c
> > +++ b/src/lxc/lxc_driver.c
> > @@ -3834,7 +3834,8 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
> >        * network's pool of devices, or resolve bridge device name
> >        * to the one defined in the network definition.
> >        */
> > -    if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
> > +    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > +        virDomainNetAllocateActualDevice(vm->def, net) < 0)
> >           return -1;
> >   
> >       actualType = virDomainNetGetActualType(net);
> > @@ -4388,7 +4389,8 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,
> >       ret = 0;
> >    cleanup:
> >       if (!ret) {
> > -        virDomainNetReleaseActualDevice(vm->def, detach);
> > +        if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> > +            virDomainNetReleaseActualDevice(vm->def, detach);
> >           virDomainNetRemove(vm->def, detachidx);
> >           virDomainNetDefFree(detach);
> >       }
> > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> > index e0729a24bf..7849aaf5b9 100644
> > --- a/src/lxc/lxc_process.c
> > +++ b/src/lxc/lxc_process.c
> > @@ -224,7 +224,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
> >                                   iface->ifname));
> >               ignore_value(virNetDevVethDelete(iface->ifname));
> >           }
> > -        virDomainNetReleaseActualDevice(vm->def, iface);
> > +        if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> > +            virDomainNetReleaseActualDevice(vm->def, iface);
> >       }
> >   
> >       virDomainConfVMNWFilterTeardown(vm);
> > @@ -558,7 +559,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
> >           if (virLXCProcessValidateInterface(net) < 0)
> >               goto cleanup;
> >   
> > -        if (virDomainNetAllocateActualDevice(def, net) < 0)
> > +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > +            virDomainNetAllocateActualDevice(def, net) < 0)
> >               goto cleanup;
> >   
> >           type = virDomainNetGetActualType(net);
> > @@ -637,7 +639,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
> >                   ignore_value(virNetDevOpenvswitchRemovePort(
> >                                   virDomainNetGetActualBridgeName(iface),
> >                                   iface->ifname));
> > -            virDomainNetReleaseActualDevice(def, iface);
> > +            if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> > +                virDomainNetReleaseActualDevice(def, iface);
> >           }
> >       }
> >       return ret;
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index 4d4ab0f375..cf37a16c64 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -4371,8 +4371,11 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> >       size_t i;
> >       int ret = -1;
> >   
> > -    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> > -        goto validate;
> > +    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Expected a interface for a virtual network"));
> > +        goto error;
> > +    }
> >   
> >       virDomainActualNetDefFree(iface->data.network.actual);
> >       iface->data.network.actual = NULL;
> > @@ -4691,7 +4694,6 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> >       if (virNetDevVPortProfileCheckComplete(virtport, true) < 0)
> >           goto error;
> >   
> > - validate:
> >       /* make sure that everything now specified for the device is
> >        * actually supported on this type of network. NB: network,
> >        * netdev, and iface->data.network.actual may all be NULL.
> > @@ -4710,19 +4712,11 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> >                 (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE &&
> >                  virtport && virtport->virtPortType
> >                  == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH))) {
> > -            if (netdef) {
> > -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -                               _("an interface connecting to network '%s' "
> > -                                 "is requesting a vlan tag, but that is not "
> > -                                 "supported for this type of network"),
> > -                               netdef->name);
> > -            } else {
> > -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -                               _("an interface of type '%s' "
> > -                                 "is requesting a vlan tag, but that is not "
> > -                                 "supported for this type of connection"),
> > -                               virDomainNetTypeToString(iface->type));
> > -            }
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                           _("an interface connecting to network '%s' "
> > +                             "is requesting a vlan tag, but that is not "
> > +                             "supported for this type of network"),
> > +                           netdef->name);
> >               goto error;
> >           }
> >       }
> By dropping the validate label, netdef is always set at this point in
> the code, so this makes sense.
>
> > @@ -4738,22 +4732,20 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> >           }
> >       }
> >   
> > -    if (netdef) {
> > -        netdef->connections++;
> > +    netdef->connections++;
> > +    if (dev)
> > +        dev->connections++;
> > +    /* finally we can call the 'plugged' hook script if any */
> > +    if (networkRunHook(obj, dom, iface,
> > +                       VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
> > +                       VIR_HOOK_SUBOP_BEGIN) < 0) {
> > +        /* adjust for failure */
> > +        netdef->connections--;
> >           if (dev)
> > -            dev->connections++;
> > -        /* finally we can call the 'plugged' hook script if any */
> > -        if (networkRunHook(obj, dom, iface,
> > -                           VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
> > -                           VIR_HOOK_SUBOP_BEGIN) < 0) {
> > -            /* adjust for failure */
> > -            netdef->connections--;
> > -            if (dev)
> > -                dev->connections--;
> > -            goto error;
> > -        }
> > -        networkLogAllocation(netdef, actualType, dev, iface, true);
> > +            dev->connections--;
> > +        goto error;
> >       }
> > +    networkLogAllocation(netdef, actualType, dev, iface, true);
> >   
> This diff is ugly but it's just unindenting an if (netdef) block
>
> So by no longer calling this function on != TYPE_NETWORK, we no longer
> get the bandwidth and vlan option validation for those interfaces. That
> seems like a minor regression. IMO not enough to hold up applying this
> but we should remember it for later. I'm pretty sure it can solved with
> some similar checks added to DomainValidate callbacks
>
> Provided there's a reason for not centralizing the TYPE_NETWORK checks:
>
>    Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx>
>
> And this could go in now IMO
>
> Thanks,
> Cole
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list