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

On 07.12.2016 09:51, Daniel P. Berrange wrote: > On Mon, Dec 05, 2016 at 02:25:25PM +0000, Daniel P. Berrange wrote: >> On Mon, Dec 05, 2016 at 03:14:50PM +0100, Michal Privoznik wrote: >>> 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. >> >> Hmm, good point. fork() is fairly fast on Linux, but it is still a >> non-negligble overhead in our startup codepath. > > I thought of a reasonable easy way we can deal with this. The key point > is that it is only a problem during domain startup where we relabel lots > of files - hotplug we're only labelling 1 file usually so overhead of > 1 fork is not an issue. Unless a disk is plugged and we are relabelling the whole backing chain. That is - we might want to use the approach you're describing in other cases too, i.e. in every hotplug.

> IOW, performance is only a problem when calling two methods > virSecuritySELinuxSetAllLabel and virSecuritySELinuxRestoreAllLabel But agreed that these two are the problem. > > What we can do is to modify those methods so that all the labelling > operations are batched up. eg have > > virSecuritySELinuxTransactionStart > virSecuritySELinuxTransactionCommit > > and when a transaction is running, we modify virSecuritySELinuxSetFileconHelper > so that it simply saves a record of the filepath + label. When we commit > the transaction, then then call the real virSecuritySELinuxSetFileconHelper > logic for the recorded files, from within our fork() helper. Yup. This would work. But then again, for now I'd go with this approach (in fact that's what I've just sent), and save this for a follow up patch - which I can work on whilst waiting for review.