On Tue, Nov 02, 2010 at 11:35:44AM -0400, Stefan Berger wrote: > During a shutdown/restart cycle libvirtd forgot the macvtap device name > that it had created on behalf of a VM so that a stale macvtap device > remained on the host when the VM terminated. Libvirtd has to actively > tear down a macvtap device and it uses its name for identifying which > device to tear down. > > The solution is to not blank out the<target dev='...'/> completely, but > only blank it out on VMs that are not active. So, if a VM is active, the > device name makes it into the XML and is also being parsed. If a VM is > not active, the device name is discarded. > > Signed-off-by: Stefan Berger&lt;stefanb(a)us.ibm.com&gt; > > --- > src/conf/domain_conf.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > Index: libvirt-acl/src/conf/domain_conf.c > =================================================================== > --- libvirt-acl.orig/src/conf/domain_conf.c > +++ libvirt-acl/src/conf/domain_conf.c > @@ -2343,7 +2343,8 @@ virDomainNetDefParseXML(virCapsPtr caps, > def->data.direct.linkdev = dev; > dev = NULL; > > - VIR_FREE(ifname); > + if ((flags& VIR_DOMAIN_XML_INACTIVE)) > + VIR_FREE(ifname); The conditional isn't required here - it is already dealt with earlier on in the file. Just remove the VIR_FREE completely.

> @@ -5801,6 +5802,8 @@ virDomainNetDefFormat(virBufferPtr buf, > virBufferAddLit(buf, "/>\n"); > virVirtualPortProfileFormat(buf, > &def->data.direct.virtPortProfile, > " "); > + if ((flags& VIR_DOMAIN_XML_INACTIVE)) > + VIR_FREE(def->ifname); This seems dubious. Formatting XML should never require changing 'def' at all.

Daniel.

On Tue, Nov 02, 2010 at 07:04:01PM -0400, Stefan Berger wrote: > On 11/02/2010 11:47 AM, Daniel P. Berrange wrote: >> On Tue, Nov 02, 2010 at 11:35:44AM -0400, Stefan Berger wrote: >>> During a shutdown/restart cycle libvirtd forgot the macvtap device name >>> that it had created on behalf of a VM so that a stale macvtap device >>> remained on the host when the VM terminated. Libvirtd has to actively >>> tear down a macvtap device and it uses its name for identifying which >>> device to tear down. >>> >>> The solution is to not blank out the<target dev='...'/> completely, but >>> only blank it out on VMs that are not active. So, if a VM is active, the >>> device name makes it into the XML and is also being parsed. If a VM is >>> not active, the device name is discarded. >>> >>> Signed-off-by: Stefan Berger&lt;stefanb(a)us.ibm.com&gt; >>> >>> --- >>> src/conf/domain_conf.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> Index: libvirt-acl/src/conf/domain_conf.c >>> =================================================================== >>> --- libvirt-acl.orig/src/conf/domain_conf.c >>> +++ libvirt-acl/src/conf/domain_conf.c >>> @@ -2343,7 +2343,8 @@ virDomainNetDefParseXML(virCapsPtr caps, >>> def->data.direct.linkdev = dev; >>> dev = NULL; >>> >>> - VIR_FREE(ifname); >>> + if ((flags& VIR_DOMAIN_XML_INACTIVE)) >>> + VIR_FREE(ifname); >> The conditional isn't required here - it is already dealt >> with earlier on in the file. Just remove the VIR_FREE >> completely. >> > The conditional further above is this code fragment here: > > } else if ((ifname == NULL)&& > xmlStrEqual(cur->name, BAD_CAST "target")) { > ifname = virXMLPropString(cur, "dev"); > if ((ifname != NULL)&& > ((flags& VIR_DOMAIN_XML_INACTIVE)&& > (STRPREFIX((const char*)ifname, "vnet")))) { > /* An auto-generated target name, blank it out */ > VIR_FREE(ifname); > } > > Unfortunately it is also testing for the prefix 'vnet'. > > In case of a macvtap device I don't want to pick up the name of the > macvtap device from the XML unless it's attached to a running domain. So > that's why I remove it above. > > If the domain went down without libvirt 'seeing' it then we miessed out > on tearing it down and in that case there may be a stale device, but I > don't support this case. So I also want to have that cleared. > > Further, I also don't accept user-provided interface names for the > reason of tear-down in case of failures while trying to start a VM. In > the failure-case it is not clear anymore whether the name was > user-provided and was previously created and needs to be torn down or > simply is a user-provided name of an interface that wasn't created and > thus should not be torn down because it may actually be clashing with > the user-provided name of a running VM. I had that before and this ended > up running danger of tearing down interfaces of running VMs when a > failure-path was invoked. Hmm, I didn't notice that macvtap didn't allow user provided interface names. IMHO this is a bug, because many users like to have predetermined names for the devices to allow easy matching up to VMs. The problem scenario you describe here with teardown killing the interface of another running guests, only occurs if the admin has configured two guests with the same macvtap device name. This is user error. We're punishing everybody by disallowing user provided names, just in case a minority create a broken config :-(

IMHO we should make this work as normal tap devices do. vnet* is a prefix for auto-generated names. Anything else is a user provided name. Regards, Daniel

On Wed, Nov 03, 2010 at 07:16:53AM -0400, Stefan Berger wrote: > On 11/03/2010 06:43 AM, Daniel P. Berrange wrote: >> On Tue, Nov 02, 2010 at 07:04:01PM -0400, Stefan Berger wrote: >>> On 11/02/2010 11:47 AM, Daniel P. Berrange wrote: >>>> On Tue, Nov 02, 2010 at 11:35:44AM -0400, Stefan Berger wrote: >>>>> During a shutdown/restart cycle libvirtd forgot the macvtap device name >>>>> that it had created on behalf of a VM so that a stale macvtap device >>>>> remained on the host when the VM terminated. Libvirtd has to actively >>>>> tear down a macvtap device and it uses its name for identifying which >>>>> device to tear down. >>>>> >>>>> The solution is to not blank out the<target dev='...'/> completely, >>>>> but >>>>> only blank it out on VMs that are not active. So, if a VM is active, the >>>>> device name makes it into the XML and is also being parsed. If a VM is >>>>> not active, the device name is discarded. >>>>> >>>>> Signed-off-by: Stefan Berger&lt;stefanb(a)us.ibm.com&gt; >>>>> >>>>> --- >>>>> src/conf/domain_conf.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> Index: libvirt-acl/src/conf/domain_conf.c >>>>> =================================================================== >>>>> --- libvirt-acl.orig/src/conf/domain_conf.c >>>>> +++ libvirt-acl/src/conf/domain_conf.c >>>>> @@ -2343,7 +2343,8 @@ virDomainNetDefParseXML(virCapsPtr caps, >>>>> def->data.direct.linkdev = dev; >>>>> dev = NULL; >>>>> >>>>> - VIR_FREE(ifname); >>>>> + if ((flags& VIR_DOMAIN_XML_INACTIVE)) >>>>> + VIR_FREE(ifname); >>>> The conditional isn't required here - it is already dealt >>>> with earlier on in the file. Just remove the VIR_FREE >>>> completely. >>>> >>> The conditional further above is this code fragment here: >>> >>> } else if ((ifname == NULL)&& >>> xmlStrEqual(cur->name, BAD_CAST "target")) { >>> ifname = virXMLPropString(cur, "dev"); >>> if ((ifname != NULL)&& >>> ((flags& VIR_DOMAIN_XML_INACTIVE)&& >>> (STRPREFIX((const char*)ifname, "vnet")))) { >>> /* An auto-generated target name, blank it out */ >>> VIR_FREE(ifname); >>> } >>> >>> Unfortunately it is also testing for the prefix 'vnet'. >>> >>> In case of a macvtap device I don't want to pick up the name of the >>> macvtap device from the XML unless it's attached to a running domain. So >>> that's why I remove it above. >>> >>> If the domain went down without libvirt 'seeing' it then we miessed out >>> on tearing it down and in that case there may be a stale device, but I >>> don't support this case. So I also want to have that cleared. >>> >>> Further, I also don't accept user-provided interface names for the >>> reason of tear-down in case of failures while trying to start a VM. In >>> the failure-case it is not clear anymore whether the name was >>> user-provided and was previously created and needs to be torn down or >>> simply is a user-provided name of an interface that wasn't created and >>> thus should not be torn down because it may actually be clashing with >>> the user-provided name of a running VM. I had that before and this ended >>> up running danger of tearing down interfaces of running VMs when a >>> failure-path was invoked. >> Hmm, I didn't notice that macvtap didn't allow user provided interface >> names. IMHO this is a bug, because many users like to have predetermined >> names for the devices to allow easy matching up to VMs. >> >> The problem scenario you describe here with teardown killing the >> interface of another running guests, only occurs if the admin has >> configured two guests with the same macvtap device name. This is >> user error. We're punishing everybody by disallowing user provided >> names, just in case a minority create a broken config :-( > I think in case of macvtap this would create more security holes than > provide a benefit. To be safe that the name of an interface is not > already used, we'd have to cross-check with all other domains' > interfaces, otherwise people will wonder what happened when a VM > couldn't start and now another VM doesn't have its interface anymore. > People do forget what names they provided to interfaces. Can multiple > users define domains on the same machine and create those clashes? If > there was an elegant solution, I'd like to support it, but for now > fixing the libvirtd's forgetfullness is more important.. also since it's > just a netto one-liner... I'm not debating that its possible to create problems in the way you describe, but AFAICT, this is no different to what can go wrong with normal TAP devices. IMHO the policy for this should be the same for macvtap and tap.

Daniel