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