[libvirt] [PATCH] network: restrict usage of port management APIs
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. This change allows us to report errors when the virtual network driver is not registered. Signed-off-by: Daniel P. Berrangé <berrange@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: -- 2.17.1
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@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:
On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote:
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'.)
Yep, this is a bigger problem than I first considered. We have four hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of them have different requirements for the way the tap devices are created. This is going to make it hard to delegate everything to the network driver, as the work is hypervisor specific. I'm trying to tackle this is distinct achieveable phases. Even the current AllocateActualDevice() method is doing too much at the same time. For example, it deals with allocating the resources inside the network driver (ie reserving a VF or hostdev), as well as populating the virDomainNetActualDef struct, and invoking hook scripts. This makes it too tangled up with the hyervisor drivers - for example needing a full domain XML is very undesirable.
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.
Even if the network driver can create tap devices, I'd still want to deal with type=network vs type=bridge separately for access control purposes. It is desirable to be able to have ACLs on the opening operation and the natural place to attach the ACL is against the virNetworkPtr object. We have no object to represent standalone bridge devices outside of a virNetworkPtr, so nothing to attach an ACL to for creating tap devices. 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 :|
On 08/09/2018 03:58 AM, Daniel P. Berrangé wrote:
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'.) Yep, this is a bigger problem than I first considered. We have four hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of
On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote: them have different requirements for the way the tap devices are created. This is going to make it hard to delegate everything to the network driver, as the work is hypervisor specific.
The differences between qemu and lxc were on my mind but, without looking, I had naively assumed that "all the others" (including libxl and bhve) were just doing something similar to qemu :-/
I'm trying to tackle this is distinct achieveable phases. Even the current AllocateActualDevice() method is doing too much at the same time. For example, it deals with allocating the resources inside the network driver (ie reserving a VF or hostdev), as well as populating the virDomainNetActualDef struct, and invoking hook scripts. This makes it too tangled up with the hyervisor drivers - for example needing a full domain XML is very undesirable.
I've always been hopeful that we could figure out some way to consolidate everything needed for setup/teardown into just a few well defined actions (to avoid an explosion of APIs that, as a side effect, are difficult to explain). But as always, "Easy to do" is easy to say (and again I find myself using ":-/") Anyway, as long as I know you're considering this, I'm sure you've got a better picture in your brain than I do, so Reviewed-by: Laine Stump <laine@laine.org> for the patch.
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. Even if the network driver can create tap devices, I'd still want to deal with type=network vs type=bridge separately for access control purposes. It is desirable to be able to have ACLs on the opening operation and the natural place to attach the ACL is against the virNetworkPtr object. We have no object to represent standalone bridge devices outside of a virNetworkPtr, so nothing to attach an ACL to for creating tap devices.
So are you saying that the network driver simply wouldn't be able to create the tap devices for type=bridge (or type=direct)? I'm not exactly following you (although I don't doubt what you say).
On Thu, Aug 09, 2018 at 11:54:42AM -0400, Laine Stump wrote:
On 08/09/2018 03:58 AM, Daniel P. Berrangé wrote:
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'.) Yep, this is a bigger problem than I first considered. We have four hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of
On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote: them have different requirements for the way the tap devices are created. This is going to make it hard to delegate everything to the network driver, as the work is hypervisor specific.
The differences between qemu and lxc were on my mind but, without looking, I had naively assumed that "all the others" (including libxl and bhve) were just doing something similar to qemu :-/
If anything libxl & bhyve are much closer to LXC in approach, in that they want a persistent TAP device passed by name, not a transient TAP device passed by FD.
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. Even if the network driver can create tap devices, I'd still want to deal with type=network vs type=bridge separately for access control purposes. It is desirable to be able to have ACLs on the opening operation and the natural place to attach the ACL is against the virNetworkPtr object. We have no object to represent standalone bridge devices outside of a virNetworkPtr, so nothing to attach an ACL to for creating tap devices.
So are you saying that the network driver simply wouldn't be able to create the tap devices for type=bridge (or type=direct)? I'm not exactly following you (although I don't doubt what you say).
That's the direction i'm leaning towards right now at least. Adding the API is easy, I just don't know what useful security model I could give it. Consider the original goal was to allow the unprivileged libvirtd to have useful network connectivity for guests. This means we want unprivileged libvirtd to be able to request a TAP device without requiring root password auth. I don't think it is viable to allow this for arbitrary bridge devices out of the box. It is, however, just about reasonable to allow unprivileged libvirtd to request a connection to the virbr0 "default" virtual network without root auth, as the connectivity provided by the dfault network is quite tightly defined. 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 :|
participants (2)
-
Daniel P. Berrangé -
Laine Stump