Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from Oct 25, 2018
190 changes: 168 additions & 22 deletions cmd/snap-discard-ns/snap-discard-ns.c
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2015 Canonical Ltd
* Copyright (C) 2015-2018 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
Expand All @@ -15,41 +15,187 @@
*
*/

#define _GNU_SOURCE

#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <fnmatch.h>
#include <limits.h>
#include <linux/magic.h>
#include <stdio.h>
#include <sys/mount.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/vfs.h>
#include <unistd.h>

#include "../libsnap-confine-private/locking.h"
#include "../libsnap-confine-private/string-utils.h"
#include "../libsnap-confine-private/utils.h"
#include "../snap-confine/ns-support.h"

#ifndef NSFS_MAGIC
#define NSFS_MAGIC 0x6e736673
#endif

int main(int argc, char **argv)
{
if (argc != 2)
die("Usage: %s snap-name", argv[0]);
const char *snap_name = argv[1];

int snap_lock_fd = sc_lock_snap(snap_name);
debug("initializing mount namespace: %s", snap_name);
struct sc_mount_ns *group =
sc_open_mount_ns(snap_name, SC_NS_FAIL_GRACEFULLY);
if (group != NULL) {
sc_discard_preserved_mount_ns(group);
sc_close_mount_ns(group);
if (argc != 2) {
printf("Usage: snap-discard-ns <SNAP-INSTANCE-NAME>\n");
return 0;
}

const char *snap_instance_name = argv[1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make sure snap instance name is sane?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/* Grab the lock holding the snap instance. This prevents races from
* concurrently executing snap-confine. The lock is explicitly released
* during normal operation but it is not preserved across the life-cycle of
* the process anyway so no attempt is made to unlock it ahead of any call
* to die() */
int snap_lock_fd = sc_lock_snap(snap_instance_name);
debug("discarding mount namespaces of snap %s", snap_instance_name);

const char *ns_dir_path = "/run/snapd/ns";
int ns_dir_fd = open(ns_dir_path, O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW);
if (ns_dir_fd < 0) {
/* The directory may legitimately not exist if no snap has started to
* prepare it. This is not an error condition. */
if (errno == ENOENT) {
return 0;
}
die("cannot open path %s", ns_dir_path);
}

/* Move to the namespace directory. This is used so that we don't need to
* traverse the path over and over in our upcoming unmount2(2) calls. */
if (fchdir(ns_dir_fd) < 0) {
die("cannot move to directory %s", ns_dir_path);
}
// Unlink the current mount profile, if any.
char profile_path[PATH_MAX] = { 0 };
sc_must_snprintf(profile_path, sizeof(profile_path),
"/run/snapd/ns/snap.%s.fstab", snap_name);
if (unlink(profile_path) < 0) {
// Silently ignore ENOENT as the profile doens't have to be there.
if (errno != ENOENT) {
die("cannot remove current mount profile: %s",
profile_path);

/* Create shell patterns that describe the things we are interested in:
*
* Preserved mount namespaces to unmount and unlink:
* - "$SNAP_INSTANCE_NAME.mnt"
* - "$SNAP_INSTANCE_NAME.[0-9]+.mnt"
*
* Applied mount profiles to unlink:
* - "snap.$SNAP_INSTANCE_NAME.fstab"
* - "snap.$SNAP_INSTANCE_NAME.[0-9]+.fstab" */
char sys_fstab_pattern[PATH_MAX];
char usr_fstab_pattern[PATH_MAX];
char sys_mnt_pattern[PATH_MAX];
char usr_mnt_pattern[PATH_MAX];
zyga marked this conversation as resolved.
Show resolved Hide resolved
sc_must_snprintf(sys_fstab_pattern, sizeof sys_fstab_pattern,
"snap\\.%s\\.fstab", snap_instance_name);
sc_must_snprintf(usr_fstab_pattern, sizeof usr_fstab_pattern,
"snap\\.%s\\.*\\.fstab", snap_instance_name);
sc_must_snprintf(sys_mnt_pattern, sizeof sys_mnt_pattern,
"%s\\.mnt", snap_instance_name);
sc_must_snprintf(usr_mnt_pattern, sizeof usr_mnt_pattern,
"%s\\.*\\.mnt", snap_instance_name);

DIR *ns_dir = fdopendir(ns_dir_fd);
zyga marked this conversation as resolved.
Show resolved Hide resolved
/* ns_dir_fd is now owned by ns_dir and will not be closed. */

while (true) {
/* Reset errno ahead of any call to readdir to differentiate errors
* from legitimate end of directory. */
errno = 0;
struct dirent *dent = readdir(ns_dir);
if (dent == NULL) {
if (errno != 0) {
die("cannot read next directory entry");
}
/* We've seen the whole directory. */
break;
}

/* We use dnet->d_name a lot so let's shorten it. */
const char *dname = dent->d_name;

/* Check the four patterns that we have against the name and set the
* two should flags to decide further actions. Note that we always
* unlink matching files so that is not reflected in the structure. */
bool should_unmount = false;
bool should_unlink = false;
struct variant {
const char *p;
zyga marked this conversation as resolved.
Show resolved Hide resolved
bool unmount;
};
struct variant variants[4] = {
{.p = sys_mnt_pattern,.unmount = true},
{.p = usr_mnt_pattern,.unmount = true},
{.p = sys_fstab_pattern},
{.p = usr_fstab_pattern},
};
for (size_t i = 0; i < sizeof variants / sizeof *variants; ++i) {
struct variant *v = &variants[i];
debug("checking if %s matches %s", dname, v->p);
int match_result = fnmatch(v->p, dname, 0);
zyga marked this conversation as resolved.
Show resolved Hide resolved
if (match_result < 0) {
die("cannot execute match against pattern %s",
v->p);
}
if (match_result == 0) {
should_unmount |= v->unmount;
should_unlink = true;
debug("file %s matches pattern %s", dname,
v->p);
/* One match is enough. */
break;
}
}

/* Stat the candidate directory entry to know what we are dealing with. */
struct stat file_info;
if (fstatat(ns_dir_fd, dname, &file_info,
AT_SYMLINK_NOFOLLOW) < 0) {
die("cannot inspect file %s", dname);
}

/* 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

continue;
}

if (should_unmount) {
/* If we are asked to unmount the file double check that it is
* really a preserved mount namespace since the error code from
* umount2(2) is inconclusive. */
int path_fd = openat(ns_dir_fd, dname,
O_PATH | O_CLOEXEC | O_NOFOLLOW);
if (path_fd < 0) {
die("cannot open path %s", dname);
}
struct statfs fs_info;
if (fstatfs(path_fd, &fs_info) < 0) {
die("cannot inspect file-system at %s", dname);
}
close(path_fd);
if (fs_info.f_type == NSFS_MAGIC
|| fs_info.f_type == PROC_SUPER_MAGIC) {
debug("unmounting %s", dname);
if (umount2(dname, MNT_DETACH | UMOUNT_NOFOLLOW)
< 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

die("cannot unmount %s", dname);
}
}
}

if (should_unlink) {
debug("unlinking %s", dname);
if (unlinkat(ns_dir_fd, dname, 0) < 0) {
die("cannot unlink %s", dname);
}
}
}

/* Close the directory and release the lock, we're done. */
if (closedir(ns_dir) < 0) {
die("cannot close directory");
}
sc_unlock(snap_lock_fd);
return 0;
}
20 changes: 14 additions & 6 deletions cmd/snap-discard-ns/snap-discard-ns.rst
Expand Up @@ -7,16 +7,16 @@ internal tool for discarding preserved namespaces of snappy applications
------------------------------------------------------------------------

:Author: zygmunt.krynicki@canonical.com
:Date: 2016-10-05
:Date: 2018-10-17
:Copyright: Canonical Ltd.
:Version: 1.0.43
:Version: 2.36
:Manual section: 5
:Manual group: snappy

SYNOPSIS
========

snap-discard-ns SNAP_NAME
snap-discard-ns SNAP_INSTANCE_NAME

DESCRIPTION
===========
Expand All @@ -43,11 +43,19 @@ FILES

`snap-discard-ns` uses the following files:

`/run/snapd/ns/$SNAP_NAME.mnt`:
`/run/snapd/ns/$SNAP_INSTNACE_NAME.mnt`:
`/run/snapd/ns/$SNAP_INSTNACE_NAME.*.mnt`:

The preserved mount namespace that is unmounted by `snap-discard-ns`.
The preserved mount namespace that is unmounted and removed by
`snap-discard-ns`. The second form is for the per-user mount namespace.

`/run/snapd/ns/snap.$SNAP_INSTNACE_NAME.fstab`:
`/run/snapd/ns/snap.$SNAP_INSTNACE_NAME.*.fstab`:

The current mount profile of a preserved mount namespace that is removed
by `snap-discard-ns`.

BUGS
====

Please report all bugs with https://bugs.launchpad.net/snap-confine/+filebug
Please report all bugs with https://bugs.launchpad.net/snapd/+filebug
60 changes: 60 additions & 0 deletions tests/main/snap-discard-ns/mount-py2.py
@@ -0,0 +1,60 @@
#!/usr/bin/env python2
"""
Pure-python limited-use wrapper around the mount library function.
"""
from __future__ import print_function, absolute_import, unicode_literals

from argparse import ArgumentParser
from ctypes import CDLL, c_char_p, c_long, get_errno
from ctypes.util import find_library
from os import strerror
from sys import stderr

__all__ = ('mount', )


class MountOpts(object):
"""MountOpts contain various flags for the mount system call."""

Bind = 4096


def mount(source, target, fstype, flags=0, data=""):
# 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not libc_name:?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

raise Exception("cannot find the C library")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make that more specific, say RuntimeError

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

libc = CDLL(libc_name, use_errno=True)
retval = libc.mount(
c_char_p(source.encode("UTF-8")),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@zyga zyga Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

c_char_p(target.encode("UTF-8")),
c_char_p(fstype.encode("UTF-8")),
c_long(flags),
None if data == "" else c_char_p(data.encode("UTF-8")))
if retval < 0:
errno = get_errno()
raise OSError(errno, strerror(errno))


def main():
# type: () -> None
parser = ArgumentParser()
parser.add_argument("source", help="path of the device or bind source")
parser.add_argument("target", help="path of the new mount point")
group = parser.add_mutually_exclusive_group(required=True)
group.add_argument("--bind", action="store_const", const="bind",
help="create a new mount point of existing path")
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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytes has no encode(), only decode()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@zyga zyga Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
mount(opts.source, opts.target, opts.fstype)


if __name__ == "__main__":
try:
main()
except Exception as err:
print(err, file=stderr)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you could

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

58 changes: 58 additions & 0 deletions tests/main/snap-discard-ns/mount-py3.py
@@ -0,0 +1,58 @@
#!/usr/bin/env python3
"""
Pure-python limited-use wrapper around the mount library function.
"""
from argparse import ArgumentParser
from ctypes import CDLL, c_char_p, c_long, get_errno
from ctypes.util import find_library
from enum import IntEnum
from os import strerror
from sys import stderr

__all__ = ('mount', )


class MountOpts(IntEnum):
"""MountOpts contain various flags for the mount system call."""

Bind = 4096


def mount(source: str, target: str, fstype: str, flags: int = 0,
data: str = "") -> None:
"""mount is a thin wrapper around the mount library function."""
libc_name = find_library("c")
if libc_name is None:
raise Exception("cannot find the C library")
libc = CDLL(libc_name, use_errno=True)
retval = libc.mount(
c_char_p(source.encode("UTF-8")),
c_char_p(target.encode("UTF-8")),
c_char_p(fstype.encode("UTF-8")),
c_long(flags),
None if data == "" else c_char_p(data.encode("UTF-8")))
if retval < 0:
errno = get_errno()
raise OSError(errno, strerror(errno))


def main() -> None:
parser = ArgumentParser()
parser.add_argument("source", help="path of the device or bind source")
parser.add_argument("target", help="path of the new mount point")
group = parser.add_mutually_exclusive_group(required=True)
group.add_argument("--bind", action="store_const", const="bind",
help="create a new mount point of existing path")
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, "", MountOpts.Bind)
else:
mount(opts.source, opts.target, opts.fstype)


if __name__ == "__main__":
try:
main()
except Exception as err:
print(err, file=stderr)
6 changes: 6 additions & 0 deletions tests/main/snap-discard-ns/mount.sh
@@ -0,0 +1,6 @@
#!/bin/sh
if [ "$(command -v python3)" != "" ]; then
exec ./mount-py3.py "$@";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else
exec ./mount-py2.py "$@";
fi