Re: [libvirt] [PATCH v1 16/21] qemu: Enter the namespace on relabelling

On 05.12.2016 14:40, Daniel P. Berrange wrote: > On Thu, Nov 24, 2016 at 03:48:05PM +0100, Michal Privoznik wrote: >> Instead of trying to fix our security drivers, we can use a >> simple trick to relabel paths in both namespace and the host. >> I mean, if we enter the namespace some paths are still shared >> with the host so any change done to them is visible from the host >> too. >> Therefore, we can just enter the namespace and call >> SetAllLabel()/RestoreAllLabel() from there. Yes, it has slight >> overhead because we have to fork in order to enter the namespace. >> But on the other hand, no complexity is added to our code. > > I'm a little concerned that this may be storing problems for us > at a later date. If the security manager classes have any state > they update such changes are invisible to the main libvirt > process now. Also stuff running between fork+exec is restricted > to async signal safe functions only. I think it is hard for us > to be entirely confident about that safety when we're running > the entire security driver labelling code in that region. > > Ultimately the only bit of the security drivers that needs to > run in the namespace is the setfilecon_raw() or chown() system > calls. > > So I wonder if we should make it possible to provide a namespace > helper callback to the security drivers that they'd use only > for the setfilecon_raw/chown calls. > > eg, we could do something like > > > typedef (*virSecurityManagerNamespaceHelperFunc)(void *opaque) > typedef (*virSecurityManagerNamespaceHelper)(virSecurityManagerNamespaceHelperFunc func, void *opaque) > > virSecurityManagerSetNamespacehelper(mgr, runInNamespaceHelper) > > ....do the labelling... > > virSecurityManagerSetNamespacehelper(mgr, NULL) > > the namespace helper would have to be stored in a thread local > since we need to cope with some VMs not having separate namespaces > > The security manager code would need todo > > struct SELinuxNamespaceData { > const char *path; > security_context_t ctx; > } > > Now instead of > > > if (setfilecon_raw(path, tcon) < 0) > ... > > it would do > > static int SELinuxNamespaceHelperFunc(void *opaque) { > struct SELinuxNamespaceData *data = opaque > > return setfilecon_raw(data->path, data->tcon) > } > > struct SELinuxNamespaceData data = { path, tcon} > > if (namespacehelper(SELinuxNamespaceHelperFunc, &data) < 0) > ... > I was thinking about this too, but I see two problems with this approach: 1) fork() on every chown() and every sefilecon_raw(). That's too much IMO.