[libvirt] [PATCH v1 07/10] lock_daemon: Implement seclabel APIs

[Michal Privoznik]

On 19.09.2014 18:13, Daniel P. Berrange wrote:
> On Wed, Sep 10, 2014 at 03:26:13PM +0200, Michal Privoznik wrote:
>> The most interesting part where all the hard work takes place.
>>
>> For storing triplets of strings a two dimensional table would be
>> the most intuitive approach. Or, if the triplet is rearranged
>> into pair of one string and a pair, a hash table can be used. But
>> hey, we already have those! Excellent. In other words,
>>
>>          <path, model, seclabel>
>>
>> is turned into:
>>
>>          <path, <model, seclabel>>
>>
>> The @path is then the key that the hash table is filled with. The
>> inner pair is turned into linked list (hash table in a hash table
>> would be awful really). Moreover, we need to store yet another
>> item: reference counter.
>
> I wonder, since 'model' is mandatory and (path, model) should
> be unique, could we not avoid the linked list complexity by
> using a string of 'model:path' as the hash key, then we only
> have a single hash item too and O(1) access to values ?
>
>
>>   int
>> -virLockDaemonRememberSeclabel(virLockDaemonPtr lockd ATTRIBUTE_UNUSED,
>> -                              const char *path ATTRIBUTE_UNUSED,
>> -                              const char *model ATTRIBUTE_UNUSED,
>> -                              const char *seclabel ATTRIBUTE_UNUSED)
>> +virLockDaemonRememberSeclabel(virLockDaemonPtr lockd,
>> +                              const char *path,
>> +                              const char *model,
>> +                              const char *seclabel)
>>   {
>> -    /* Implement me */
>> -    return -1;
>> +    int ret = -1;
>> +    virSeclabelListPtr list = NULL;
>> +    virSeclabelListPtr tmp = NULL;
>> +    bool list_created = false;
>> +
>> +    virMutexLock(&lockd->lock);
>> +
>> +    if (!(list = virHashLookup(lockd->seclabels, path))) {
>> +        if (VIR_ALLOC(list) < 0)
>> +            goto cleanup;
>> +
>> +        if (virHashAddEntry(lockd->seclabels, path, list) < 0)
>> +            goto cleanup;
>> +
>> +        list_created = true;
>> +    }
>> +
>> +    tmp = list;
>> +    if (!(tmp = virSeclabelListFind(list, model))) {
>> +        /* seclabel for @model doesn't exit yet */
>> +        if (!(tmp = virSeclabelListAdd(list, model, seclabel)))
>> +            goto cleanup;
>> +    }
>> +
>> +    /* Update refcount */
>> +    ret = ++tmp->item->refcount;
>> +
>> + cleanup:
>> +    virMutexUnlock(&lockd->lock);
>> +    if (ret < 0 && list_created) {
>> +        virHashRemoveEntry(lockd->seclabels, path);
>> +        virSeclabelListFree(list);
>> +    }
>> +    return ret;
>>   }
>
> What concerns me a little is that we don't have any association between
> the label, use count and the running VMs that triggered the need for the
> relabelling.
>
> Lets say that a virtualization host with VMs running crashes. The libvirtd
> and virtlocked on that host will loose all state. That's fine currently
> since you're not sharing the data across hosts. We're going to need to
> share the data though, so when we do that sharing, I think we're going
> to need to remember information about the VMs + hosts associated with
> the label, so that when a host crashes, we can have a chance of purging
> the now stale information.

I see.

>
> Now, as discussed on IRC it isn't critical to support multi-host mode
> right away, but this does impact on the RPC protocol, since the messages
> we are passing do not include any information about the VM or host.
>
> So I'm thinking that this series should record the VM name + uuid and
> the host name + uuid against the seclabels and plumb that through the
> RPC protocol at least.

Right, this would be great.

>
> If we did that now, I think we might not even need to add new RPC
> protocol for lockd, and instead just re-use the existing protocol
> messages and at least some of the client/dispatcher code too.
> It would be more like a case of just setting up a new lockspace
> for recording info about labels, instead of a completely new set
> of infrastructure side-by-side.

I was thinking about this prior to starting new implementation. The 
problem I had found myself in was that in secdrivers we use pure 
virDomainDefPtr but the VM's pid is stored in virDomainObjPtr. And I 
don't think we want to refactor all the code just to tunnel the vm->pid 
into function actually calling chown() or fsetfilecon_raw(). Or do we?

Michal