Re: [libvirt] [PATCH v3 04/13] security_manager: Rework metadata locking

On 10/17/2018 11:45 AM, Daniel P. Berrangé wrote: > On Wed, Oct 17, 2018 at 11:06:46AM +0200, Michal Privoznik wrote: >> Trying to use virlockd to lock metadata turns out to be too big >> gun. Since we will always spawn a separate process for relabeling >> we are safe to use thread unsafe POSIX locks and take out >> virtlockd completely out of the picture. > > Note safety of POSIX locks is not merely about other threads. > > If *any* code we invoke from security_dac or security_selinux > were to open() & close() the file we're labelling we'll > loose the locks. > > This is a little concerning since we call out to a few functions > in src/util that might not be aware they should *never* open)( > and close() the path they're given. It looks like we lucky at > the moment, but this has future gun -> foot -> ouch potential. > > Likewise we also call out to libselinux APIs. I've looked at > the code for the APIs we call and again, right now, we look > to be safe, but there's some risk there. > > I'm not sure whether this is big enough risk to invalidate this > forking approach or not though. This kind of problem was the > reason why virtlockd was created to hold locks in a completely > separate process from the code with the critical section. But unless we check for hard links virtlockd will suffer from the POSIX locks too. For instance, assume that B is a hardlink to A. Then, if a process opens A, locks it an the other opens B and closes it the lock is released. Even though A and B look distinct.

The problem with virlockd is that our current virlockspace impl is not aware of different offsets. We can try to make it so, but that would complicate the code a lot. Just imagine that there's a domain already running and it holds disk content lock over file C. If an user wants to start a domain (which too has disk C), libvirtd will try to lock C again (even though on different offset) but it will fail to do so because C is already locked.

Another problem with virtlockd that I faced was that with current implementation the connection to virlockd is opened in metadataLock(), where the connection FD is dup()-ed to prevent connection closing. The connection is then closed in metadataUnlock() where the dup()-ed FD is closed. This works pretty much good, except for fork(). If a fork() occurs between metadataLock() and metadataUnlock() the duplicated FD is leaked into the child and we can't be certain when will the child close it. The worst case scenario is that the child will close the FD when we are holding some resources, which results in virtlockd killing libvirtd (= registered owner of the locks).

Alternatively, we can switch to OFD locks and have the feature work only on Linux. If anybody from BSD users will want the feature they will have to push their kernel developers to implement some sensible file locking. Also, one of the questions is what we want metadata locking to be. So far I am aiming on having exclusive access to the file that we are chown()-ing (or setfcon()-ing) so that I can put a code around it that manipulates XATTR where the original owner would be stored. This way the metadata lock is held only for a fraction of a second. I guess we don't want the metadata lock be held through whole run of a domain, i.e. obtained at domain start and released at domain shutdown. I don't see much benefit in that.