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

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. Avoiding the fork would mean having some persistent child and RPC to it which is also likely to add just as much overhead. I guess we'll just have to bear in mind the limitations for security managers in future and go with what you've done.