| Message ID | 20250303064209.602577-1-naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/run_privatens: check if it is mount point for --make-private | expand |
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 > >
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 >> >>
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 >>> >>>
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 > >>> > >>>
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
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(-)