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