[libvirt] [PATCH] macvtap: libvirtd forgot macvtap device name during a shutdown/restart cycle

[Stefan Berger]

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 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 at 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.
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