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/ :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
