cmd/snap-discard-ns: add support for per-user mount namespaces #6010

zyga · 2018-10-17T13:48:11Z

This patch rewrites snap-discard-ns to support user mounts. This
involves scanning for the new per-user mount namespaces and mount
profiles.

The program was simplified to not use the "abstraction" of provided by
"ns-support.h". The abstraction was poor and with the upcoming changes
to snap-confine to accommodate per-user mount namespaces it is easier to
audit and understand the code directly. This change makes "ns-support.h"
private to snap-confine so it can evolve separately.

The manual page was updated to reflect the new functionality and refresh
some cruft like project bug report URL.

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

This patch rewrites snap-discard-ns to support user mounts. This involves scanning for the new per-user mount namespaces and mount profiles. The program was simplified to not use the "abstraction" of provided by "ns-support.h". The abstraction was poor and with the upcoming changes to snap-confine to accommodate per-user mount namespaces it is easier to audit and understand the code directly. This change makes "ns-support.h" private to snap-confine so it can evolve separately. The manual page was updated to reflect the new functionality and refresh some cruft like project bug report URL. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

This patch fixes an issue that only manifests itself on Ubuntu 14.04 where /etc/mtab is a real file and not a symbolic link to /proc/mtab. On such system mount(1) which uses libmount internally, keeps track of all the operations by editing the mtab file. Sadly, our hackery inside snap-confine and snap-discard-ns is totally not editing mtab. This causes confusion when umount(1) is asked to detach a mount namespace file. The problem is that at that time the file is already unmounted, thanks to updated snap-discard-ns. Because the test had atificially created the .mnt file with touch and mount(1) there is now a stale mount entry in mtab. In the cleanup section of each test (actually the prepare section but conceptually it is the cleanup after previous test) we, via snapd.postrm or snap-mgmt.sh unmount and remove all the preserved mount namespaces. When umount(1) is given a file that is not a mount point it goes to search mtab, treating it as a mount _source_ (not target) and proceeds to umount the target of whatever is found. To avoid this issue, since we are not using libmount in snap-discard-ns or snap-confine, simply avoid using libmount driven mount(1) in the test as well. This patch implements a thin python wrapper around mount and uses it for the construction of the fake per-user mount namespace. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

On Amazon Linux 2 we don't have Python 3 but we do have Python 2. Implement a Python 2.7 compatible version of mount.py and use it automatically if Python 3 is not available. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

cmd/snap-discard-ns/snap-discard-ns.c

bboozzoo · 2018-10-19T15:34:21Z

tests/main/snap-discard-ns/mount-py2.py

+    # type: (unicode, unicode, unicode, int, unicode) -> None
+    """mount is a thin wrapper around the mount library function."""
+    libc_name = find_library(b"c")
+    if libc_name is None:


if not libc_name:?

PEP8 suggests testing None the way I did to avoid masking the distinction between None and other things that evaluate to false in boolean context.

bboozzoo · 2018-10-19T15:37:23Z

tests/main/snap-discard-ns/mount.sh

@@ -0,0 +1,6 @@
+#!/bin/sh
+if [ "$(command -v python3)" != "" ]; then
+	exec ./mount-py3.py "$@";


Can we have just one version of the script? The way you've written py2 version, it should work with py3 just fine.

I tried to initially but in the end I decided no to. Using one version of the script puts most pressure on the unicode vs string vs bytes across the two versions. Right now both versions are correct but this could be subtly broken with a single script.

I think one script is enough. With the exception of b"", I don't see anything wrong with py2 version that would blow up under py3.

bboozzoo · 2018-10-19T15:41:02Z

tests/main/snap-discard-ns/mount-py2.py

+    try:
+        main()
+    except Exception as err:
+        print(err, file=stderr)


Did you forget to raise? This will mask any errors, even programming ones.

I didn't intend to show the traceback but I think I did forget to return the exit status. I will fix this.

I think you can then drop the whole try/except block and just call main() or catch say OSError and let the others fall through. Right now you'll catch stuff like NameError which is probably not very useful without the actual backtrace.

Then you could

try (OSError, RuntimeError) as err: print(err, file=stderr) raise SystemExit(1)

Thanks to Maciej for spotting this. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

bboozzoo · 2018-10-24T07:22:30Z

tests/main/snap-discard-ns/mount-py2.py

+    try:
+        main()
+    except Exception as err:
+        print(err, file=stderr)


I think you can then drop the whole try/except block and just call main() or catch say OSError and let the others fall through. Right now you'll catch stuff like NameError which is probably not very useful without the actual backtrace.

bboozzoo · 2018-10-24T07:25:13Z

tests/main/snap-discard-ns/mount-py2.py

+    """mount is a thin wrapper around the mount library function."""
+    libc_name = find_library(b"c")
+    if libc_name is None:
+        raise Exception("cannot find the C library")


Maybe make that more specific, say RuntimeError

I switched error handling, it should work fine as-is now.

bboozzoo · 2018-10-24T07:26:56Z

tests/main/snap-discard-ns/mount-py2.py

+    try:
+        main()
+    except Exception as err:
+        print(err, file=stderr)


Then you could

try (OSError, RuntimeError) as err: print(err, file=stderr) raise SystemExit(1)

bboozzoo · 2018-10-24T07:35:23Z

tests/main/snap-discard-ns/mount-py2.py

+    group.add_argument("-t", "--type", help="filesystem type to mount")
+    opts = parser.parse_args()
+    if opts.bind is not None:
+        mount(opts.source, opts.target, b"", MountOpts.Bind)


mount(opts.source, opts.target, "", MountOpts.Bind)? If you run this under Py3, b"".encode('utf-8') when calling mount will fail.

What is the problem with b"".encode('utf-8')?

bytes has no encode(), only decode()

In python 2 bytes is just a string. It does have the encode method.
The b prefix makes it a plain string, not an unicode string. Note that this is the py2 version of the code.

Having said that I will look at unifying this even though I would much rather not have to use python 2 and all of its quirks at all.

What I meant is that if you use the Py2 version of the script in Py3, b"" is bytes which does not have encode(). I think it's the only line here that can cause compatibility problems.

IIRC u"" is the unicode string in Py2, b"" and "" are the same (str). On the other hand, in Py3 u"" is the same as "" (str), but not b"" (which is bytes).

Yeah but this was for Python 2 specifically. The Python 3 version did not need that.

EDIT, removed part about u"" on Python3.

Please look at what I did now, it works and is Mypy-clean on both versions.

bboozzoo · 2018-10-24T07:48:20Z

tests/main/snap-discard-ns/mount.sh

@@ -0,0 +1,6 @@
+#!/bin/sh
+if [ "$(command -v python3)" != "" ]; then
+	exec ./mount-py3.py "$@";


I think one script is enough. With the exception of b"", I don't see anything wrong with py2 version that would blow up under py3.

bboozzoo · 2018-10-24T07:49:15Z

tests/main/snap-discard-ns/mount-py2.py

+        raise Exception("cannot find the C library")
+    libc = CDLL(libc_name, use_errno=True)
+    retval = libc.mount(
+            c_char_p(source.encode("UTF-8")),


utf-8 is the default, so you may drop that.

UTF-8 is not the default, defaults depend on locale.

I think it is in Py3. In Py2 it was derived from your locale or sth. OTOH, I'm not sure we should be forcing utf-8 here which may not be the same as input encoding so the input path provided to the script will be a different byte sequence from one encoded to utf-8.

bboozzoo · 2018-10-24T11:06:49Z

tests/main/snap-discard-ns/mount.sh

+#!/bin/sh
+if [ "$(command -v python3)" != "" ]; then
+	exec python3 ./mount.py "$@";
+elif [ "$(command -v python3)" != "" ]; then


bboozzoo · 2018-10-24T11:07:03Z

tests/main/snap-discard-ns/mount.sh

+if [ "$(command -v python3)" != "" ]; then
+	exec python3 ./mount.py "$@";
+elif [ "$(command -v python3)" != "" ]; then
+	exec python2 ./mount-py2.py "$@";


s/mount-py2/mount/

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

stolowski

Looks good, but a couple of questions.

cmd/snap-discard-ns/snap-discard-ns.c

stolowski · 2018-10-25T09:38:24Z

cmd/snap-discard-ns/snap-discard-ns.c

-			die("cannot remove current mount profile: %s",
-			    profile_path);
+
+	const char *snap_instance_name = argv[1];


Should we make sure snap instance name is sane?

Good point, we probably should. I will make that adjustment.

cmd/snap-discard-ns/snap-discard-ns.c

stolowski · 2018-10-25T09:42:29Z

cmd/snap-discard-ns/snap-discard-ns.c

+		/* We are only interested in regular files. The .mnt files, even if
+		 * bind-mounted, appear as regular files and not as symbolic links due
+		 * to the peculiarities of the Linux kernel. */
+		if (!S_ISREG(file_info.st_mode)) {


Would it make sense to move this up & check early before trying to match?

I wrote this watching strace logs and realised we are stat-ing things that we don't care about. The pattern match is much faster than a system call so I chose to order it this way around.

stolowski · 2018-10-25T09:47:59Z

cmd/snap-discard-ns/snap-discard-ns.c

+			    || fs_info.f_type == PROC_SUPER_MAGIC) {
+				debug("unmounting %s", dname);
+				if (umount2(dname, MNT_DETACH | UMOUNT_NOFOLLOW)
+				    < 0) {


This would read better if < 0) { was on the same line where the if starts, but I presume it was formatted automatically like this? Slightly odd given that the comment block above is much wider.

This is automatically formatted, the mystery of indent. Perhaps given that this a near-full rewrite I could use clang-format for this file and actually switch to a sane format profile?

That might be a good idea... Feel free to give it a try before landing and apply if it produces sensible output.

stolowski · 2018-10-25T09:51:58Z

tests/main/snap-discard-ns/mount.sh

+if [ "$(command -v python3)" != "" ]; then
+	exec python3 ./mount.py "$@";
+elif [ "$(command -v python2)" != "" ]; then
+	exec python2 ./mount.py "$@";


Why this logic if it works with both versions of python, why not simply check for /usr/bin/python (and have #!/usr/bin/env python shebang?

python is usually python2 but I wanted to be explicit. In practice we will only pick up the other case (python2) on the Amazon Linux 2 systems.

Fair enough, not a problem at all, just looks a little elaborate considering the fact that we don't actually care which python version it is.

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

stolowski

Thanks for the changes, looks good to me! +1

stolowski · 2018-10-25T11:11:42Z

cmd/snap-discard-ns/snap-discard-ns.c

+	const char *snap_instance_name = argv[1];
+	struct sc_error *err = NULL;
+	sc_instance_name_validate(snap_instance_name, &err);
+	sc_die_on_error(err);


Nice, thank you.

stolowski · 2018-10-25T11:14:38Z

cmd/snap-discard-ns/snap-discard-ns.c

+			    || fs_info.f_type == PROC_SUPER_MAGIC) {
+				debug("unmounting %s", dname);
+				if (umount2(dname, MNT_DETACH | UMOUNT_NOFOLLOW)
+				    < 0) {


That might be a good idea... Feel free to give it a try before landing and apply if it produces sensible output.

stolowski · 2018-10-25T11:15:36Z

tests/main/snap-discard-ns/mount.sh

+if [ "$(command -v python3)" != "" ]; then
+	exec python3 ./mount.py "$@";
+elif [ "$(command -v python2)" != "" ]; then
+	exec python2 ./mount.py "$@";


Fair enough, not a problem at all, just looks a little elaborate considering the fact that we don't actually care which python version it is.

Since this is a "clean-slate" due to the rewrite we can introduce alternate formatting mode. After some experimentation I picked clang-format and used the following style: {BasedOnStyle: Google, IndentWidth: 4, ColumnLimit: 120} This makes the code read better than with defaults, especially due to longer column limit. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga force-pushed the feature/user-mount-ns-2 branch 2 times, most recently from 302bed0 to 9d4c97d Compare October 17, 2018 14:18

zyga force-pushed the feature/user-mount-ns-2 branch from 9d4c97d to 24d0a12 Compare October 17, 2018 17:28

zyga added 2 commits October 18, 2018 19:43

zyga requested a review from bboozzoo October 18, 2018 20:15

bboozzoo reviewed Oct 19, 2018

View reviewed changes

zyga added 2 commits October 22, 2018 14:27

cmd/snap-discard-ns: make sure fdopendir worked

b83f92d

Thanks to Maciej for spotting this. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

tests: return non-zero if unmount fails

d268415

Thanks to Maciej for spotting this. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

bboozzoo reviewed Oct 24, 2018

View reviewed changes

bboozzoo approved these changes Oct 24, 2018

View reviewed changes

zyga force-pushed the feature/user-mount-ns-2 branch from 2d6ad9d to 08c352b Compare October 24, 2018 11:28

zyga added 2 commits October 24, 2018 13:30

tests: unify mount.py for Python 2 and 3

65e0b22

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

tests: catch and handle only OSError

236eb62

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

zyga force-pushed the feature/user-mount-ns-2 branch from 08c352b to 236eb62 Compare October 24, 2018 11:30

stolowski reviewed Oct 25, 2018

View reviewed changes

zyga added 4 commits October 25, 2018 13:01

cmd/snap-discard-ns: check for FNM_NOMATCH

f315dae

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

cmd/snap-discard-ns: don't abbreviate pattern

1b16aa3

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

cmd/snap-discard-ns: validate snap instance name

95f32ac

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

cmd/snap-discard-ns: add comment about PATH_MAX

3201251

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

stolowski approved these changes Oct 25, 2018

View reviewed changes

zyga merged commit 5decc83 into snapcore:master Oct 25, 2018

cmd/snap-discard-ns: add support for per-user mount namespaces #6010

cmd/snap-discard-ns: add support for per-user mount namespaces #6010

Conversation

zyga commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga Oct 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga Oct 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga Oct 24, 2018 •

edited

zyga Oct 24, 2018 •

edited