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

[Daniel P. Berrange]

On Wed, Dec 07, 2016 at 11:19:46AM +0100, Michal Privoznik wrote:
> 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.

True - most apps use fairly shallow chains, so probably not too
critical but worth optimizing if its not too hard.

> > 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.

Sounds fine with me

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|