[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gentoo-portage-dev
Subject:    Re: [gentoo-portage-dev] [PATCH 2/3] Support FEATURES=pid-sandbox
From:       Michał_Górny <mgorny () gentoo ! org>
Date:       2018-11-18 8:25:14
Message-ID: 1542529514.1293.7.camel () gentoo ! org
[Download RAW message or body]


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" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]