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

Daniel P. Berrange berrange at redhat.com
Mon Dec 5 14:25:25 UTC 2016


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.

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/ :|




More information about the libvir-list mailing list