[libvirt] [PATCH] network: restrict usage of port management APIs
Laine Stump
laine at laine.org
Thu Aug 9 03:41:23 UTC 2018
On 08/08/2018 11: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, so virt
> drivers must not delegation validation to it for NICs with
> type!=network.
Although I never thought through all the minute details to the end (and
didn't need to because ,,,AllocateActualDevice() wasn't in a public
API), I had always figured that these calls into the network driver
would be where, eventually, we would do all of the plumbing for a
network device, like creating the tap/macvtap device during startup, and
disconnecting/deleting devices during domain shutdown. (it also kind of
makes sense for nwfilters to be added/removed by the network driver
during these APIs, since the filterref is in the <interface> definition).
That's the reason for the unconditional calls.
(one of the "minute details" that I hadn't thought about was the fact
that we probably can't do *all* of the plumbing at one time - at the
very least we need to have one API call for creating the devices and
hooking them up, and another call that happens right before the guest
CPUs are started (to bring everything online). Still, if we limit the
netAllocate API to only being called for type='network' then we
definitely will need an additional API that will likely be called
unconditionally just after netAllocation is called just for type='network'.)
BTW, once it is the responsibility of the network driver to create tap
devices and connect them to bridges, the network driver will no longer
be optional (unless we want to duplicate the code in the hypervisor
drivers), but that is the price we have to pay in order to give
unprivileged libvirt the ability to use any type of network device.
>
> This change allows us to report errors when the virtual network driver
> is not registered.
>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> src/conf/domain_conf.c | 12 +++++-------
> src/libxl/libxl_domain.c | 6 ++++--
> src/libxl/libxl_driver.c | 9 ++++++---
> src/lxc/lxc_driver.c | 6 ++++--
> src/lxc/lxc_process.c | 10 ++++++++--
> src/network/bridge_driver.c | 22 +++++++++++++++-------
> src/qemu/qemu_hotplug.c | 17 +++++++++++------
> src/qemu/qemu_process.c | 9 ++++++---
> 8 files changed, 59 insertions(+), 32 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index adcd8f41b9..e7d2acdcc9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -29947,13 +29947,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);
> }
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 2ab78ac9a5..c78d5ee96c 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -791,7 +791,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);
> }
> }
>
> @@ -948,7 +949,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 5a5e792957..fb5f046ade 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3264,7 +3264,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);
> @@ -3314,7 +3315,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;
> @@ -3737,7 +3739,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 8867645cdc..8729fc0174 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -3871,7 +3871,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);
> @@ -4425,7 +4426,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 33c806630b..7a6b40d9b8 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -213,7 +213,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);
> @@ -547,6 +548,10 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
> if (virLXCProcessValidateInterface(net) < 0)
> goto cleanup;
>
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> + virDomainNetAllocateActualDevice(def, net) < 0)
> + goto cleanup;
> +
> if (virDomainNetAllocateActualDevice(def, net) < 0)
> goto cleanup;
>
> @@ -626,7 +631,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 f92cc61e47..c44cb73c5b 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4458,8 +4458,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;
> @@ -4778,7 +4781,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.
> @@ -4881,8 +4883,11 @@ networkNotifyActualDevice(virDomainDefPtr dom,
> size_t i;
> char *master = NULL;
>
> - if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> - return;
> + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Expected a interface for a virtual network"));
> + goto error;
> + }
>
> obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
> if (!obj) {
> @@ -5114,8 +5119,11 @@ networkReleaseActualDevice(virDomainDefPtr dom,
> size_t i;
> int ret = -1;
>
> - if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> - return 0;
> + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Expected a interface for a virtual network"));
> + goto error;
> + }
>
> obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
> if (!obj) {
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1488f0a7c2..512fead050 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1062,7 +1062,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr 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);
> @@ -1352,7 +1353,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>
> virDomainNetRemoveHostdev(vm->def, net);
>
> - virDomainNetReleaseActualDevice(vm->def, net);
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> + virDomainNetReleaseActualDevice(vm->def, net);
> }
>
> VIR_FREE(nicstr);
> @@ -3722,7 +3724,8 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>
> /* this function doesn't work with HOSTDEV networks yet, thus
> * no need to change the pointer in the hostdev structure */
> - virDomainNetReleaseActualDevice(vm->def, olddev);
> + if (olddev->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> + virDomainNetReleaseActualDevice(vm->def, olddev);
> virDomainNetDefFree(olddev);
> /* move newdev into the nets list, and NULL it out from the
> * virDomainDeviceDef that we were given so that the caller
> @@ -3753,7 +3756,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
> * that the changes were minor enough that we didn't need to
> * replace the entire device object.
> */
> - if (newdev)
> + if (newdev && newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> virDomainNetReleaseActualDevice(vm->def, newdev);
>
> return ret;
> @@ -4310,7 +4313,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
> virDomainHostdevDefFree(hostdev);
>
> if (net) {
> - virDomainNetReleaseActualDevice(vm->def, net);
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> + virDomainNetReleaseActualDevice(vm->def, net);
> virDomainNetDefFree(net);
> }
>
> @@ -4406,7 +4410,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>
> qemuDomainNetDeviceVportRemove(net);
>
> - virDomainNetReleaseActualDevice(vm->def, net);
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> + virDomainNetReleaseActualDevice(vm->def, net);
> virDomainNetDefFree(net);
> ret = 0;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c4e33723d1..440e2b326d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3127,7 +3127,8 @@ qemuProcessNotifyNets(virDomainDefPtr def)
> if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT)
> ignore_value(virNetDevMacVLanReserveName(net->ifname, false));
>
> - virDomainNetNotifyActualDevice(def, net);
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> + virDomainNetNotifyActualDevice(def, net);
> }
> }
>
> @@ -5326,7 +5327,8 @@ qemuProcessNetworkPrepareDevices(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)
> goto cleanup;
>
> actualType = virDomainNetGetActualType(net);
> @@ -7075,7 +7077,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>
> /* kick the device out of the hostdev list too */
> virDomainNetRemoveHostdev(def, net);
> - virDomainNetReleaseActualDevice(vm->def, net);
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> + virDomainNetReleaseActualDevice(vm->def, net);
> }
>
> retry:
More information about the libvir-list
mailing list