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

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(a)redhat.com&gt; >>> --- >>> 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.