Re: [PATCH] tools/run_privatens: check if it is mount point for --make-private

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Mar 04, 2025 at 01:38:48AM +0000, Naohiro Aota wrote:
> On Mon Mar 3, 2025 at 10:29 PM JST, Naohiro Aota wrote:
> > On Mon Mar 3, 2025 at 7:10 PM JST, Zorro Lang wrote:
> >> On Mon, Mar 03, 2025 at 03:42:09PM +0900, Naohiro Aota wrote:
> >>> While /tmp is mounted with tmpfs in the most setup, it still can be a non-mount
> >>> point. For example, I'm running the fstests in a container, which does not
> >>> mount /tmp inside the container.
> >>> 
> >>> Running any test case on such system results in having the following error
> >>> printed, which leads to all the test cases fail due to the output difference.
> >>> 
> >>>   mount: /tmp: not mount point or bad option.
> >>>          dmesg(1) may have more information after failed mount system call.
> >>> 
> >>> These lines are printed by the "mount --make-private" command. So, fix that by
> >>> using mountpoint command to check if the directory is a mount point or not.
> >>> 
> >>> Fixes: 247ab01fa227 ("check: run tests in a private pid/mount namespace")
> >>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> >>> ---
> >>>  tools/run_privatens | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>> 
> >>> diff --git a/tools/run_privatens b/tools/run_privatens
> >>> index df94974ab30c..c52e0128b8f9 100755
> >>> --- a/tools/run_privatens
> >>> +++ b/tools/run_privatens
> >>> @@ -8,7 +8,7 @@
> >>>  
> >>>  if [ -n "${FSTESTS_ISOL}" ]; then
> >>>  	for path in /proc /tmp; do
> >>> -		mount --make-private "$path"
> >>> +		mountpoint "$path" >/dev/null && mount --make-private "$path"
> >>
> >> Oh, if /tmp isn't a mountpoint on your system, don't you need to think about ...
> >>
> >>>  	done
> >>>  	mount -t proc proc /proc
> >>>  	mount -t tmpfs tmpfs /tmp
> >>         ^^^^^^^^^^^^^^^^^^^^^^^^^
> >>
> >> ... this step? I think the first run_privatens process will remain a "mounted"
> >> /tmp on your system.
> >>
> >> And then later tests will use this tmpfs. That's nearly equal to "We always
> >> make sure there's a tmpfs mount on /tmp at the beginning of fstests running".
> >
> > That does not happen because we are already in a mount namespace created
> > by nsexec. The mounted "/tmp" is local to each test, and each test
> > showed the error above.
> >
> >>
> >> But if we don't run that "mount tmpfs" step, I think it's not what this script
> >> wants (to isolate the data in /tmp). Right?
> >
> > I guess we can do these instead?
> >
> > mount --make-private -t proc proc /proc
> > mount --make-private -t tmpfs tmpfs /tmp
> >
> > Actually, I'm not quite confindent that we really need "--make-private"
> > here. As we mount new instance of proc and tmpfs and we don't bind them,
> > I don't see much point making them private.
> 
> No, I was wrong. We still need to make them private, not to propagate
> "mount tmpfs" or "mount proc" to the outside. If not, we will see new
> instance of tmpfs mounted on /tmp in the PID 1 environment, which
> breaks several applications :(.
> 
> So, in the end, I think my patch did it correctly, no?

Yes.  The "mount --make-private" prohibits propagation of changes to
this mount outside of the mount namespace.  The "mount -t tmpfs tmpfs
/tmp" creates a new tmpfs that is not exposed to the outside world.

This should've been documented better, Zorro could you please add a few
comments on commit:

	if [ -n "${FSTESTS_ISOL}" ]; then
		# Don't allow changes to these mountpoints to propagate
		# outside of our mount namespace...
		for path in /proc /tmp; do
			mountpoint "$path" >/dev/null && \
				mount --make-private "$path"

		done

		# ...because here we create new mounts that are private
		# to this mount namespace that we don't want to escape.
		mount -t proc proc /proc
		mount -t tmpfs tmpfs /tmp
	fi

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> >
> >>
> >> Thanks,
> >> Zorro
> >>
> >>
> >>
> >>> -- 
> >>> 2.48.1
> >>> 
> >>>