[libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling

[Michal Privoznik]

On 08/20/2018 07:17 PM, Michal Prívozník wrote:
> On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
>> On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
>>> Fortunately, we have qemu wrappers so it's sufficient to put
>>> lock/unlock call only there.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>  src/qemu/qemu_security.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 107 insertions(+)
>>>


>> I'm wondering if this is really the right level to plug in the metadata
>> locking ?  What about if we just pass a virLockManagerPtr to the security
>> drivers and let them lock each resource at the time they need to modify
>> its metadata. This will trivially guarantee that we always lock the exact
>> set of files that we'll be changing metadata on.
>>
>> eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
>> would directly call virLockManagerAcquire & virLockManagerRelease,
>> avoiding the entire virDomainLock  abstraction which was really
>> focused around managing the content locks around lifecycle state
>> changes.
>>
> 
> Yeah, I vaguely recall writing code like this (when I was trying to
> solve this some time ago). Okay, let me see if that's feasible.
> 
> But boy, this is getting hairy.

So as I'm writing these patches I came to realize couple of things:

a) instead of domain PID we should pass libvirtd PID because we want to
release the locks if libvirtd dies not domain.

b) that, however, leads to a problem because
virLockManagerLockDaemonAcquire() closes the socket to virtlockd causing
it to kill the owner of the lock, i.e. it kills libvirtd immediately.

c) even if I hack around b) so that we connect only once for each
lock+unlock pair call, we would still connect dozens of times when
starting a domain (all the paths we label times all active secdrivers).
So we should share connection here too.

Now question is how do do this effectively and cleanly (code-wise). For
solving b) we can have a new flag, say VIR_LOCK_MANAGER_KEEP_CONNECT
that would cause  virLockManagerLockDaemonAcquire() to save the
(connection, program, counter) tuple somewhere into lock driver private
data so that virLockManagerLockDaemonRelease() called with the same flag
can re-use the data.

For c) I guess we will need to open the connection in
virLockManagerLockDaemonNew(), put the socket FD into event loop so that
EOF is caught and initiate reopen in that case. However, I'm not sure if
this is desirable - to be constantly connected to virtlockd.

Michal