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

On Thu, Aug 23, 2018 at 04:54:01PM +0200, Michal Privoznik wrote: > 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. This is fine ;-P > 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. Yeah, makes sense. > 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. Can we use a model similar to what I did for the shared secondary driver connections. By default a call to virGetConnectNetwork() will open a new connection. To avoid opening & closing 100's of connections though, some places will call virSetConnectNetwork() to store a pre-opened connection in a thread local. That stays open until virSetConnectNetwork is called again passing in a NULL. We would put such a cache around any bits of code that triggers many connection calls to virlockd.