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

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

[Naohiro Aota]

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"
 	done
 	mount -t proc proc /proc
 	mount -t tmpfs tmpfs /tmp
-- 
2.48.1

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

[Zorro Lang]

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

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?

Thanks,
Zorro



> -- 
> 2.48.1
> 
> 

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

[Naohiro Aota]

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.

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

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

[Naohiro Aota]

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?

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

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

[Darrick J. Wong]

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