Add support module for namespace sharing

[zyga]

This patch adds a module that implements mount namespace sharing. It is
not yet used by snap-confine. The module implements a set of library
functions that can perform the necessary magic.

As the topic is a bit unexpected a brief description follows.

Linux namespaces can be created with the unshare(2) call. They are
represented as files in the nsfs filesystem under /proc/$pid/ns/. The
mount namespace is represented as /proc/$pid/ns/mnt. An open file
descriptor to a file like that can be used along with setns(2) to move a
thread to a different namespace.

While namespaces are normally bound to the lifetime of a given process
they can be preserved by bind-mounting the appropriate namespace file to
another location. This feature serves as the basis for the namespace
sharing feature.

The mount namespace is a little bit special as it requires some
additional things to be in place before the bind mount can happen. All
violations of the rules listed below results in mount failing with
EINVAL.

In order to preserve the mount namespace the calling process must be in
a different mount namespace (snap-confine just uses the original namespace in
which it executes). This can be achieved by forking a child process that
can see its parent mount namespace file in /proc/$ppid/ns/mnt and having
the parent process move to a new namespace by calling unshare(). The
destination of the bind mount must be on a filesystem that is mounted
without any peers (in the sense of shared subtrees). This can be checked
by inspecting and parsing the /proc/self/mountinfo file. The new module
includes support function that bind mounts the target directory over
itself and the converts it to a private mount.

Actual mount namespaces are kept in /run/snapd/ns (this is also the
directory that gets converted to a private mount). The actual namespaces
are put in "groups" with names identical to the snap name. In practice
the preserved namespaces are in /run/snapd/ns/$SNAP_NAME.mnt

In order to make everything race-free the library uses locking based on
flock(2). There are two lock files. One global, required to make
/run/snapd/ns a private mount, and one local, required to manipulate a
particular mount namespace. The locks are /run/snapd/ns/.lock and
/run/snaps/ns/$SNAP_NAME.lock respectively.

Everything is coded defensively, terminating the process in case
something bad happens. The child process of snap-confine is using
prctl() to ensure it gets killed when the parent process dies for any
reason.

One notable thing that is not present in this patch is the adjusted
apparmor profile for snap-confine and for its child process (it switches
hats to a new profile for added paranoia and security). It will be
presented along with a, hopefully small, patch that enables namespace
sharing in practice.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

[jdstrand]

I spent some time looking at this today and will comment inline. I can say that I may have further comments once the PR for using these functions is in place. There is a lot of 'you should only do this if that' language in the header that I think makes this module somewhat error prone to use. The header does do a good job of explaining each function though. Perhaps it should at the top also give steps on the ordering of how the functions are intended to be used?

[jdstrand]

Perhaps: "As of version 1.0.41 all the applications from the same snap will share the same mount namespace. Applications from different snaps continue to use separate mount namespaces."

[zyga]

Done

[jdstrand]

FYI, I found the .lock file under-documented. I'm not sure where it would be best to document it, but it wasn't part of the google doc spec and I had to put it together myself why it was needed.

[zyga]

I've improved the documentation, have a look of that is clear now.

[jdstrand]

Can you implement this?

[zyga]

Yes, I just found glib lacking in performing this thing in one, nice call.

[zyga]

Done

[jdstrand]

Nice test :)

[jdstrand]

For cleanliness, shouldn't we LOCK_UN after this assert?

[zyga]

Closing the file descriptor unlocks the lock. We could do it but at some point there's no need to do it.

[jdstrand]

<sys/types.h> is specified twice.

[zyga]

Fixed

[jdstrand]

Perhaps change this to "We use 'const char *' so we can update sc_ns_dir in the testsuite"

[zyga]

Done

[jdstrand]

I think this could be worded a bit better. Eg:

// Read /proc/self/mountinfo and check if /run/snapd/ns is a private bind mount.
//
// We do this because /run/snapd/ns cannot be shared with any other peers as per:
// https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt

Also, why // here and /** up above?

[zyga]

I moved from vm to vm and I used misconfigure vim. I'll update the docs to match.

[zyga]

Done

[jdstrand]

I tend to prefer < 0 checks instead of == -1. I won't block if this stays as is. Note, you use == -1 in quite a few places.

[zyga]

I realised I made this mistake just before pushing this. I was on edge between < 0 and -1 and I think I'll go back to < 0 as they cannot be mistyped to = -1 and make sense in any way.

[zyga]

Fixed all of them to be < 0

[jdstrand]

Moving this down above dir_fd = open(... would be slightly more clear.

[zyga]

Done

[jdstrand]

I would probably prefer this to be declared above sc_initialize_ns_groups(), but I won't block on it.

[zyga]

Done

[jdstrand]

Perhaps a comment stating that the caller must call sc_close_ns_group() to close dir_fd and lock_fd would be clearer.

[zyga]

Hmm? Wait, this is not the same lock file. This is the global lock file that only this function ever touches.

[jdstrand]

I'm not sure there is anything to be done about it, but is seems that sc_initialize_ns_groups() is going to be called on every app invocation, which means every invocation will try to create sc_ns_dir, create the lock file and parse the mountinfo table. Since /run is tmpfs we don't have to worry about media wear and performance should be ok, plus this only has to be done when the app is launched, not for every fork/exec the app does, but thought I'd at least point it out explicitly.

[zyga]

I'm aware of this and I was wondering if there's any way that we can avoid this without introducing some kind of raciness. I guess we could do it in a job that runs before we mount, say, the core snap, but at some point this becomes a bit distant and magic and I was worried that the essential property for being able to preserve the mount namespace would get lost.

[jdstrand]

Can have more than just 'shared:xxx'. Perhaps say: // If /run/snapd/ns has no optional fields, we know it is mounted private and there is nothing else to do.

[zyga]

Done

[jdstrand]

This strcmp() is perfectly correct for today's parse_mountinfo_entry(), but if parse_mountinfo_entry() ever changes, we'll have a bug here. Can you add a comment to parse_mountinfo_entry() in mountinfo.c circa line 238 that we 'strcpy(entry->optional_fields, "");' to guarantee consumers have a non-null string?

[zyga]

Yep, thanks for pointing this out

[zyga]

Done

[jdstrand]

I was surprised to see event_fd based on the spec, but I don't think it is a problem.

[zyga]

It is easier than a pipe and more reliable (race free) than a signal handler.

[jdstrand]

I know that you calloc'd this and so child is already zero, but an explicit assignment with a comment that you check for non-zero in other functions to verify the mount namespace capture process' status would make this a bit clearer.

[zyga]

Done

[tyhicks]

On 09/06/2016 04:17 PM, jdstrand wrote:

In src/ns-support.c
#126 (comment):

+void sc_initialize_ns_groups()
+{

• int dir_fd attribute ((cleanup(sc_cleanup_close))) = -1;
• debug("creating namespace group directory %s", sc_ns_dir);
• mkpath(sc_ns_dir);
• debug("opening namespace group directory %s", sc_ns_dir);
• dir_fd = open(sc_ns_dir, O_DIRECTORY | O_PATH | O_CLOEXEC | O_NOFOLLOW);
• if (dir_fd == -1) {

•    die("cannot open namespace group directory");
  

• }
• debug("opening lock file for group directory");
• int lock_fd attribute ((cleanup(sc_cleanup_close))) = -1;
• lock_fd = openat(dir_fd,

•         SC_NS_LOCK_FILE,
  

•         O_CREAT | O_RDWR | O_CLOEXEC | O_NOFOLLOW, 0600);
  

Please also add O_EXCL to this openat() call (at which point, you don't
need to specify O_NOFOLLOW).

I don't think that a lockfile should be opened with O_EXCL. Requiring
O_EXCL defeats the purpose of the lockfile since the exclusivity
provided by the lockfile is enforced by flock() once you've opened the file.

(Note that I have not reviewed the code in this PR at all but saw this
review comment fly by and thought I'd comment on it)

[jdstrand]

Heh, of course you are right there. :) I've deleted those two comments.

[jdstrand]

I fear we may be in an 'undefined' area here. The actual full path is sc_ns_dir + '/' + lock_fname so I'm not sure what side-effects there might be if lock_name actually was PATH_MAX - 1. Really this need only be the maximum of SNAP_NAME + .lock + '\0', but I realize we don't want to be that tightly coupled with snapd's definition of how long SNAP_NAME can be. Perhaps define this length as PATH_MAX - length of sc_ns_dir + '/' + '\0'?

[jdstrand]

Might be nice to check if dir_fd, lock_fd and event_fd are all '>= 0' here. I like to also check the return code of close(), but the dir_fd and the lock_fd aren't being written to and event_fd is only written to trigger an event, so not checking should be safe.

[zyga]

They don't have to be >= though, e.g event_fd is only used if required.

[jdstrand]

It would be nice if sc_lock_ns_mutex() and sc_unlock_ns_mutex() both checked if lock_fd was '>=0'.

[zyga]

Done, with unit tests as well

[jdstrand]

Can you add a comment here on why you are not using O_EXCL? AIUI, you want to either open the nsfs mounted file or open the newly created file. It would be good to document that for auditability.

[zyga]

Yes, you are right both in the deduction of the intent and the suggestion to document this better.

[zyga]

Documented

[jdstrand]

Per the setns() man page, EINVAL might be returned for a bunch of reasons. The comment should be updated and the code reviewed to make sure the assumptions regarding EINVAL are still valid.

[zyga]

You are right. I plan to change this a little to call fstatfs() to determine if the opened file descriptor belongs to nsfs or not. This will let us predictably die() when anything goes wrong.

[jdstrand]

This sounds like a good idea.

[jdstrand]

s/it//

[zyga]

Fixed

[jdstrand]

Why aa_change_hat() instead of aa_change_profile()? This seems like a one way operation for the child so aa_change_profile() may be better. Also, I think you can drop -helper since it implies a helper binary (at least to me anyway).

[zyga]

No particular reason, the profile name looked nice.

What should be the profile name here? snap-confine//mount-namespace-capture or just mount-namespace-capture?

[jdstrand]

Depends on the policy. I suggest you use a child profile so it is clear this is for snap-confine. Eg:

@LIBEXECDIR@/snap-confine (attach_disconnected) {
    ...
    profile mount-namespace-capture (attach_disconnected) {
        ...
    }
}

With this you would use @LIBEXECDIR@/snap-confine//mount-namespace-capture.

[zyga]

I will follow up with a small branch that does this. Thanks

[jdstrand]

A comment on how casting pid to int is ok with glibc would be good here.

[zyga]

Done

[jdstrand]

This should be '< 0'.

[zyga]

Fixed

[jdstrand]

This should be '< 0'.

[zyga]

Fixed

[jdstrand]

This should be '< 0'.

[zyga]

Fixed

[jdstrand]

Typo: should be flock

[zyga]

Fixed

[jdstrand]

'is not locked in any way' is confusing since this is a lock file. Perhaps say, 'is not locked until sc_lock_ns_mutex() is called'. Also, it seems a bit odd that the lock file is created here but not locked until another function. Perhaps document why that is?

[zyga]

Fixed

[jdstrand]

sc_preserve_ns_group() is not a thing, I think you mean sc_preserve_populated_ns_group()

[jdstrand]

For completeness, please also add spread tests once there are consumers for sc_initialize_ns_groups(), sc_create_or_join_ns_group(), sc_should_populate_ns_group(), sc_preserve_populated_ns_group() and sc_discard_preserved_ns_group().

[jdstrand]

There is a space after this asterisk. Can you remove it?

[zyga]

Thank you for the review. It's a bit too late to do this responsibly but I will go over all the feedback and ensure it is addressed. Depending on how you feel I can propose a sister branch that uses this module to implement fully-working mount namespace sharing, along with a few spread tests for the basic property.

[jdstrand]

This may make sense to move above the aa_change_* call, but I'm curious as to the affect on policy it will have on the parent and child profiles. I'm not terribly worried about the timing of the call-- in the unlikely event the parent dies before this call (which is not attacker controlled), the child will become a zombie and at some point init should reap it so even if the parent is killed before prctl, we should be ok.

[zyga]

Let's postpone changing this until the next pull request where we actually start to use this (and we can see the apparmor profile).

[jdstrand]

pid < 0?

[zyga]

Fixed

[zyga]

Fixed

[jdstrand]

Typo: sc_should_populate_ns_group()

[zyga]

Hmm?

[jdstrand]

Sorry, please disregard this. You are correct.

[zyga]

Resolved on IRC

[jdstrand]

Oh, scary. I hope there are no bugs in the test code otherwise the developer might have severe data loss....

[jdstrand]

Can we do some checks on dir to make sure it is inside the test dir?

[jdstrand]

Alternatively remove the files we created and them rmdir the directory. If we make rmdir() fail the test, then we can catch stray files added via code changes easily. I'd prefer this approach.

[zyga]

I tried to use the function that doesn't do shell expansion but glib is just annoying here. I'll try again.

[zyga]

I've implemented this a little bit better now. I think it good to merge now.

[jdstrand]

There is a typo here. Perhaps be a little more explicit though. Eg: // NOTE: This ensures that optional_fields is never NULL. If this changes, must adjust all callers of parse_mountinfo_entry() accordingly.

[zyga]

Fixed

[jdstrand]

Thanks for all the changes! I think this is ok to merge provided:

• my feedback from today is addressed (ie, pid < 0, rm_rf, comment changes)
• your proposed fstatfs() is implemented

Don't feel like you need to implement the apparmor policy changes just yet as you mentioned, but let's not cut a new version of snap-confine until the other bits are in place.

[jdstrand]

This is soo much nicer, thanks! :)

It's interesting that NSFS_MAGIC is not included in man fstatfs. Note that this was added only in http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/include/uapi/linux/magic.h?id=e149ed2b805fefdccf7ccdfc19eca22fdd4514ac so this will likely need to be added to the backports list if it isn't already. It does seem present in the 14.04 Ubuntu kernel.

[zyga]

I've informed tvoss about this. With the sanity check we'll know if tests are all green. Thanks for making me aware of this :)

[jdstrand]

Good thinking! :)