[libvirt] [PATCH] macvtap: libvirtd forgot macvtap device name during a shutdown/restart cycle
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 <stefanb@us.ibm.com> --- 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); break; @@ -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); break; case VIR_DOMAIN_NET_TYPE_USER:
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 <stefanb@us.ibm.com>
--- 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. -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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<stefanb@us.ibm.com>
--- 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. So, in case a ifname is given in case of macvtap (type 'direct') it means it was started before and needs to be torn down. After every tear-down the ifname is also free()d.
@@ -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.
Let me remove it and test it only with the modification above kept as-is. Stefan
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<stefanb@us.ibm.com>
--- 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 -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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<stefanb@us.ibm.com>
--- 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
On 11/03/2010 06:43 AM, Daniel P. Berrange wrote: 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... Stefan
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<stefanb@us.ibm.com>
--- 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 -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 11/03/2010 07:21 AM, Daniel P. Berrange wrote:
On Wed, Nov 03, 2010 at 07:16:53AM -0400, Stefan Berger 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<stefanb@us.ibm.com>
--- 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
On 11/03/2010 06:43 AM, Daniel P. Berrange wrote: 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. I believe a solution to support user-provide macvtap device names would be to:
- introduce a boolean in the direct device structure indicating whether the device was created, i.e., should be torn down in a failure path - the function delMacvtapTap receives a boolean indicating whether it is invoked as part of a failure path; - if not a failure path, then the VM must have been started successfully and this is part of a detach or VM shutdown when we tear down the device blindly; this also covers the case when the boolean above was lost due to a libvirtd restart - if in a failure path, then the VM must have the boolean in the direct device structure set to be torn down; the failure path is invoked also as part of invoking qemudShutdownVMDaemon, but that function also seems to be invoked as part of proper cleanup -> now it would have to have a boolean indicating what the reason is for its invocation, i.e., get a 'true' at least when invoked in the cleanup path of qemudStartVMDaemon() to indicate the failure path. How does this sound? Stefan
Daniel
participants (2)
-
Daniel P. Berrange -
Stefan Berger