Re: [libvirt] [PATCH RFC 1/3] security_dac: Remember owner prior chown() and restore on relabel

On 28.02.2013 02:22, Eric Blake wrote: > On 02/27/2013 02:25 AM, Michal Privoznik wrote: >>> Are you really planning on storing a string uid:gid? Wouldn't it be >>> simpler to store a uid_t and gid_t as read from struct stat, as long as >>> the data is only in memory? And when storing the data to disk in XML to >>> survive libvirtd restarts, it seems like storing two attributes >>> user='nnn' group='nnn' is nicer than storing one attribute >>> owner='+nnn:+nnn' that requires further parsing back into user and group. >> >> My idea is; store userName:groupName whenever possible; When one of them >> is not accessible, use +NNN instead. The rationale is - whenever a user >> or a gropu changes its ID, we will follow it. For instance, a file X has >> owner A:B (1:1) but libvirt chowns to C:D. Meanwhile A's ID is changed >> from 1 to 2. So when relabeling we should chown to 2:1 instead of 1:1 as >> A's ID is 2 not 1. That means I have to do parsing and all the >> virAsprintf magic. However, maybe this is not what we want and I should >> really remember just numeric values of IDs which has nice tradeoff - >> much simpler code. > > Migration and shared storage makes this problem so much tougher - the > uid on shared storage is common, but the name is not necessarily common. > I'd go for uid only; leave name lookups to each local machine > connecting to shared storage, but don't store the name ourselves. > Besides, admins tend not to change the name associated with a uid all > that frequently (it's generally one-time setup). > > Dan has a point that you really need to involve the lock manager or use > some persistent storage (extended attribute or additional file in the > storage pool) alongside the file whose attributes we want to remember - > if a file is on shared storage, then it is the responsibility of the > last machine using the image to restore permissions, even if it was not > the machine that first did a chmod(). Merely storing a hash in just a > single libvirtd instance won't scale. Does our virlockd interface > support attaching attributes to a file as part of locking it? Okay. You two has a point. I have not thought about that initially. Anyway, if I go with xattrs, would it be safe? I mean, I'd need to atomically get extended attribute for refCount. If there's none, get current owner of the file and store it - again - as an extended attribute among with refCount = 1. Any later libvirtd instance would then just atomically increment/decrement refCount. And here's where I see the trouble. In case of files protected by sanlock/virlockd we are safe - the relabeling is done after we obtain the lock. But what about files that are not protected by lock, e.g. <shared/> and <readonly/>? Unless we lock them for the time we do the xattr magic, we cannot guarantee atomicity, right? But this will extend need of usage of sanlock/virlockd to nearly all cases, which is something we might not want. Even if you have a couple of domains within a single host and you're sharing an ISO image you'd need the lock daemon.