1 |
On Sat, 2018-11-17 at 19:20 -0800, Zac Medico wrote: |
2 |
> On 11/14/18 12:02 AM, Michał Górny wrote: |
3 |
> > @@ -531,6 +543,15 @@ def _exec(binary, mycommand, opt_name, fd_pipes, env, gid, groups, uid, umask, |
4 |
> > errno.errorcode.get(ctypes.get_errno(), '?')), |
5 |
> > noiselevel=-1) |
6 |
> > else: |
7 |
> > + if unshare_pid: |
8 |
> > + # pid namespace requires us to become init |
9 |
> > + # TODO: do init-ty stuff |
10 |
> > + # therefore, fork() ASAP |
11 |
> > + fork_ret = os.fork() |
12 |
> > + if fork_ret != 0: |
13 |
> > + pid, status = os.waitpid(fork_ret, 0) |
14 |
> > + assert pid == fork_ret |
15 |
> > + os._exit(status) |
16 |
> > if unshare_mount: |
17 |
> > # mark the whole filesystem as private to avoid |
18 |
> > # mounts escaping the namespace |
19 |
> > @@ -541,6 +562,18 @@ def _exec(binary, mycommand, opt_name, fd_pipes, env, gid, groups, uid, umask, |
20 |
> > # TODO: should it be fatal maybe? |
21 |
> > writemsg("Unable to mark mounts private: %d\n" % (mount_ret,), |
22 |
> > noiselevel=-1) |
23 |
> > + if unshare_pid: |
24 |
> > + if mount_ret != 0: |
25 |
> > + # can't proceed without private mounts |
26 |
> > + os._exit(1) |
27 |
> |
28 |
> For the benefit of anyone not watching |
29 |
> https://github.com/gentoo/portage/pull/379, the mount_ret is expected |
30 |
> to be non-zero inside a chroot where `mount --make-rprivate /` would |
31 |
> fail because / is not a mountpoint, and this is an extremely valuable |
32 |
> use case for tools like catalyst that perform builds inside a chroot. |
33 |
> |
34 |
> Based on /proc/[pid]/mountinfo documentation in the proc(5) manpage, |
35 |
> and empirical analysis of /proc/self/mountinfo in various states, |
36 |
> it should be reasonable to assume that the current propagation flags |
37 |
> are suitable if there is not a shared / or /proc mountpoint found in |
38 |
> /proc/self/mountinfo. We can use a function like this to check if the |
39 |
> `mount --make-rprivate /` call is needed: |
40 |
> |
41 |
> def want_make_rprivate(): |
42 |
> try: |
43 |
> with open('/proc/self/mountinfo', 'rb') as f: |
44 |
> if re.match(rb'^\S+ \S+ \S+ \S+ (?P<mountpoint>/|/proc) \S+ (?:\S+:\S+ )*(?P<shared>shared:\S+ )', f.read(), re.MULTILINE) is None: |
45 |
> return False |
46 |
> except EnvironmentError: |
47 |
> pass |
48 |
> return True |
49 |
|
50 |
Technically speaking, FEATURES=mount-sandbox goes beyond /proc, so |
51 |
making it guess based on the state of /proc is wrong. Yes, we need only |
52 |
/proc for the purpose of pid-sandbox but we shouldn't presume the user |
53 |
wouldn't want more. |
54 |
|
55 |
Maybe we should split the logic more, and e.g. be more fatal about |
56 |
--make-rprivate failing when mount-sandbox is explicitly used, and only |
57 |
care about /proc when pid-sandbox is only used. In the latter case, it |
58 |
probably doesn't make sense to check but just try to --make-private |
59 |
/proc. /proc is naturally a mountpoint, so it should cause no harm to |
60 |
privatize it. |
61 |
|
62 |
Also, after some extra reading I think we should use 'slave' instead of |
63 |
'private'. Having things implicitly 'private' might be surprising since |
64 |
user's actions won't propagate into the build namespace, and I think we |
65 |
want them to propagate. |
66 |
|
67 |
-- |
68 |
Best regards, |
69 |
Michał Górny |