[LTP] [PATCH v1] move_mount03: check allow to mount beneath top mount

[LTP] [PATCH v1] move_mount03: check allow to mount beneath top mount

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
---
 include/lapi/fsmount.h                        |  4 ++
 runtest/syscalls                              |  1 +
 .../kernel/syscalls/move_mount/.gitignore     |  1 +
 .../kernel/syscalls/move_mount/move_mount03.c | 63 +++++++++++++++++++
 4 files changed, 69 insertions(+)
 create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c

diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
index 07eb42ffa..216e966c7 100644
--- a/include/lapi/fsmount.h
+++ b/include/lapi/fsmount.h
@@ -115,6 +115,10 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
 }
 #endif /* HAVE_MOUNT_SETATTR */
 
+#ifndef MOVE_MOUNT_BENEATH
+#define MOVE_MOUNT_BENEATH 		0x00000200
+#endif /* MOVE_MOUNT_BENEATH */
+
 /*
  * New headers added in kernel after 5.2 release, create them for old userspace.
 */
diff --git a/runtest/syscalls b/runtest/syscalls
index b1125dd75..04b758fd9 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
 
 move_mount01 move_mount01
 move_mount02 move_mount02
+move_mount03 move_mount03
 
 move_pages01 move_pages01
 move_pages02 move_pages02
diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
index 83ae40145..ddfe10128 100644
--- a/testcases/kernel/syscalls/move_mount/.gitignore
+++ b/testcases/kernel/syscalls/move_mount/.gitignore
@@ -1,2 +1,3 @@
 /move_mount01
 /move_mount02
+/move_mount03
diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
new file mode 100644
index 000000000..071fd984c
--- /dev/null
+++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Wei Gao <wegao@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Verify allow to mount beneath top mount
+ *
+ */
+
+#include <stdio.h>
+
+#include "tst_test.h"
+#include "lapi/fsmount.h"
+#include "lapi/sched.h"
+
+#define MNTPOINT "mntpoint"
+#define DIRA "LTP_DIR_A"
+
+static void run(void)
+{
+	int fd;
+
+	SAFE_UNSHARE(CLONE_NEWNS);
+	SAFE_MKDIR(DIRA, 0777);
+	SAFE_TOUCH(DIRA "/A", 0, NULL);
+	/* The parent mnt should be private if check MOVE_MOUNT_BENEATH */
+	SAFE_MOUNT("none", "/", "none", MS_REC | MS_PRIVATE, NULL);
+	SAFE_MOUNT(DIRA, DIRA, "none", MS_BIND, NULL);
+	fd = SAFE_OPEN(DIRA, O_RDONLY | O_DIRECTORY);
+
+	TST_EXP_PASS(move_mount(fd, "", AT_FDCWD, MNTPOINT,
+				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH));
+
+	if (access(MNTPOINT "/A", F_OK) == 0)
+		tst_res(TFAIL, "mount beneath top mount failed");
+	else
+		tst_res(TPASS, "mount beneath top mount pass");
+
+	if (tst_is_mounted_at_tmpdir(MNTPOINT))
+		SAFE_UMOUNT(MNTPOINT);
+
+	if (access(MNTPOINT "/A", F_OK) == 0)
+		tst_res(TPASS, "mount beneath top mount pass");
+	else
+		tst_res(TFAIL, "mount beneath top mount failed");
+
+	remove(DIRA "/A");
+	SAFE_RMDIR(DIRA);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.needs_root = 1,
+	.format_device = 1,
+	.mntpoint = MNTPOINT,
+	.mount_device = 1,
+	.all_filesystems = 1,
+	.skip_filesystems = (const char *const []){"fuse", NULL},
+	.min_kver = "6.5.0"
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] move_mount03: check allow to mount beneath top mount

[Richard Palethorpe]

Hello,

Wei Gao via ltp <ltp@lists.linux.it> writes:

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  include/lapi/fsmount.h                        |  4 ++
>  runtest/syscalls                              |  1 +
>  .../kernel/syscalls/move_mount/.gitignore     |  1 +
>  .../kernel/syscalls/move_mount/move_mount03.c | 63 +++++++++++++++++++
>  4 files changed, 69 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c
>
> diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
> index 07eb42ffa..216e966c7 100644
> --- a/include/lapi/fsmount.h
> +++ b/include/lapi/fsmount.h
> @@ -115,6 +115,10 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
>  }
>  #endif /* HAVE_MOUNT_SETATTR */
>  
> +#ifndef MOVE_MOUNT_BENEATH
> +#define MOVE_MOUNT_BENEATH 		0x00000200
> +#endif /* MOVE_MOUNT_BENEATH */
> +
>  /*
>   * New headers added in kernel after 5.2 release, create them for old userspace.
>  */
> diff --git a/runtest/syscalls b/runtest/syscalls
> index b1125dd75..04b758fd9 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
>  
>  move_mount01 move_mount01
>  move_mount02 move_mount02
> +move_mount03 move_mount03
>  
>  move_pages01 move_pages01
>  move_pages02 move_pages02
> diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
> index 83ae40145..ddfe10128 100644
> --- a/testcases/kernel/syscalls/move_mount/.gitignore
> +++ b/testcases/kernel/syscalls/move_mount/.gitignore
> @@ -1,2 +1,3 @@
>  /move_mount01
>  /move_mount02
> +/move_mount03
> diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
> new file mode 100644
> index 000000000..071fd984c
> --- /dev/null
> +++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Verify allow to mount beneath top mount

This should at least reference the following commit:

commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
Author: Christian Brauner <brauner@kernel.org>
Date:   Wed May 3 13:18:42 2023 +0200

    fs: allow to mount beneath top mount

To be honest I am struggling to understand what all of this
does. However I think that I found some issues with the test which I
have noted below.

> + *
> + */
> +
> +#include <stdio.h>
> +
> +#include "tst_test.h"
> +#include "lapi/fsmount.h"
> +#include "lapi/sched.h"
> +
> +#define MNTPOINTR "mntpoint"
> +#define DIRA "LTP_DIR_A"
> +
> +static void run(void)
> +{
> +	int fd;
> +
> +	SAFE_UNSHARE(CLONE_NEWNS);
> +	SAFE_MKDIR(DIRA, 0777);
> +	SAFE_TOUCH(DIRA "/A", 0, NULL);
> +	/* The parent mnt should be private if check MOVE_MOUNT_BENEATH */
> +	SAFE_MOUNT("none", "/", "none", MS_REC | MS_PRIVATE, NULL);

Is it necessary and safe to do this on root? It's not clear what effect
setting all mounts under the present "/" and namespace might have. I
guess it should be possible to create a mount to use as the parent that
is private?

> +	SAFE_MOUNT(DIRA, DIRA, "none", MS_BIND, NULL);
> +	fd = SAFE_OPEN(DIRA, O_RDONLY | O_DIRECTORY);

Why do we open this directory? It seems both source and destination can
be paths.

Also it is never closed.

> +
> +	TST_EXP_PASS(move_mount(fd, "", AT_FDCWD, MNTPOINT,
> +				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH));
> +
> +	if (access(MNTPOINT "/A", F_OK) == 0)

So we have mounted mntpoint/LTP_DIR_A at mntpoint (or moved it there),
but we don't expect file A to be there?

> +		tst_res(TFAIL, "mount beneath top mount failed");
> +	else
> +		tst_res(TPASS, "mount beneath top mount pass");

This message doesn't explain what the test is doing. You can probably
use TST_EXP_* here as well.

> +
> +	if (tst_is_mounted_at_tmpdir(MNTPOINT))
> +		SAFE_UMOUNT(MNTPOINT);
> +
> +	if (access(MNTPOINT "/A", F_OK) == 0)
> +		tst_res(TPASS, "mount beneath top mount pass");
> +	else
> +		tst_res(TFAIL, "mount beneath top mount failed");
> +

So if MNTPOINT is not under tmpdir we do nothing, but the test now
expects A to exist?

If we always unmount MNTPOINT then this should always fail because we
never created anything at mntpoint/A.

> +	remove(DIRA "/A");

This should probably be SAFE_UNLINK.

> +	SAFE_RMDIR(DIRA);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.needs_root = 1,
> +	.format_device = 1,
> +	.mntpoint = MNTPOINT,
> +	.mount_device = 1,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *const []){"fuse", NULL},
> +	.min_kver = "6.5.0"
> +};



-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] move_mount03: check allow to mount beneath top mount

[Wei Gao via ltp]

On Tue, Nov 14, 2023 at 09:17:15AM +0000, Richard Palethorpe wrote:
> Hello,
> 
> Wei Gao via ltp <ltp@lists.linux.it> writes:
> 
> This should at least reference the following commit:
> 
> commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
> Author: Christian Brauner <brauner@kernel.org>
> Date:   Wed May 3 13:18:42 2023 +0200
> 
>     fs: allow to mount beneath top mount
> 
> To be honest I am struggling to understand what all of this
> does. However I think that I found some issues with the test which I
> have noted below.
> 
Thanks for your pateint review, i also spend a lot of time to recall
my memory for this test case, so i decide to reconstruct this test case
with heavy comments to introduce how this feature work and give every
explaination for each step of test case.
> 
> 
> 
> -- 
> Thank you,
> Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2] move_mount03: check allow to mount beneath top mount

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
---
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/move_mount/.gitignore     |   1 +
 .../kernel/syscalls/move_mount/move_mount03.c | 111 ++++++++++++++++++
 3 files changed, 113 insertions(+)
 create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c

diff --git a/runtest/syscalls b/runtest/syscalls
index b1125dd75..04b758fd9 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
 
 move_mount01 move_mount01
 move_mount02 move_mount02
+move_mount03 move_mount03
 
 move_pages01 move_pages01
 move_pages02 move_pages02
diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
index 83ae40145..ddfe10128 100644
--- a/testcases/kernel/syscalls/move_mount/.gitignore
+++ b/testcases/kernel/syscalls/move_mount/.gitignore
@@ -1,2 +1,3 @@
 /move_mount01
 /move_mount02
+/move_mount03
diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
new file mode 100644
index 000000000..dadb19178
--- /dev/null
+++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Christian Brauner <brauner@kernel.org>
+ * Copyright (c) 2023 Wei Gao <wegao@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Verify allow to mount beneath top mount base following commit:
+ * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
+ * Author: Christian Brauner <brauner@kernel.org>
+ * Date:   Wed May 3 13:18:42 2023 +0200
+ *     fs: allow to mount beneath top mount
+ *
+ * Above commit has heavily commented but i found following commit
+ * contain simple summary of this feature for easy understanding:
+ *
+ * commit c0a572d9d32fe1e95672f24e860776dba0750a38
+ * Author: Linus Torvalds <torvalds@linux-foundation.org>
+ *       TL;DR:
+ *
+ *         > mount -t ext4 /dev/sda /mnt
+ *           |
+ *           --/mnt    /dev/sda    ext4
+ *
+ *         > mount --beneath -t xfs /dev/sdb /mnt
+ *           |
+ *           --/mnt    /dev/sdb    xfs
+ *             --/mnt  /dev/sda    ext4
+ *
+ *         > umount /mnt
+ *           |
+ *           --/mnt    /dev/sdb    xfs
+ *
+ * So base above scenario design following scenario for LTP check:
+ *
+ *         > mount -t tmpfs /DIRA
+ *           |
+ *           --/DIRA(create A file within DIRA)
+ *
+ *         > mount -t tmpfs /DIRB
+ *           |
+ *           --/DIRA(create B file within DIRB)
+ *
+ *         > move_mount --beneath /DIRA /DIRB
+ *           |
+ *           --/mnt    /DIRA /DIRB
+ *             --/mnt  /DIRB
+ *
+ *         If you check content of /DIRB, you can see file B
+ *
+ *         > umount /DIRB
+ *           |
+ *           --/mnt    /DIRA /DIRB
+ *         Check content of /DIRB, you can see file A exist since
+ *         current /DIRB mount source is already become /DIRA
+ *
+ * More detail can be found in following link:
+ * Link: https://lwn.net/Articles/930591/
+ * Link: https://github.com/brauner/move-mount-beneath
+ */
+
+#include <stdio.h>
+
+#include "tst_test.h"
+#include "lapi/fsmount.h"
+#include "lapi/sched.h"
+
+#define DIRA "LTP_DIR_A"
+#define DIRB "LTP_DIR_B"
+
+static void run(void)
+{
+	int fda, fdb;
+
+	SAFE_MKDIR(DIRA, 0777);
+	SAFE_MKDIR(DIRB, 0777);
+	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
+	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
+	SAFE_TOUCH(DIRA "/A", 0, NULL);
+	SAFE_TOUCH(DIRB "/B", 0, NULL);
+
+	TEST(fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE));
+
+	if (fda == -1) {
+		tst_res(TFAIL | TTERRNO, "open_tree() failed");
+		return;
+	}
+
+	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
+	TST_EXP_PASS(move_mount(fda, "", fdb, "",
+				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
+				MOVE_MOUNT_T_EMPTY_PATH));
+	SAFE_CLOSE(fda);
+	SAFE_CLOSE(fdb);
+
+	TST_EXP_PASS(access(DIRB "/B", F_OK));
+	SAFE_UMOUNT(DIRB);
+	TST_EXP_PASS(access(DIRB "/A", F_OK));
+
+	SAFE_UMOUNT(DIRB);
+	SAFE_UMOUNT(DIRA);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.needs_root = 1,
+	.min_kver = "6.5.0",
+	.needs_tmpdir = 1,
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] move_mount03: check allow to mount beneath top mount

[Petr Vorel]

Hi Wei,

I suppose there was no v1, right?

-i2 fails, please fix it.

# move_mount03 -i2
move_mount03.c:79: TINFO: Mounting none to /tmp/LTP_movcovMfK/LTP_DIR_A fstyp=tmpfs flags=0
move_mount03.c:80: TINFO: Mounting none to /tmp/LTP_movcovMfK/LTP_DIR_B fstyp=tmpfs flags=0
move_mount03.c:92: TPASS: move_mount(fda, "", fdb, "", MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH | MOVE_MOUNT_T_EMPTY_PATH) passed
move_mount03.c:98: TPASS: access(DIRB "/B", F_OK) passed
move_mount03.c:99: TINFO: Umounting /tmp/LTP_movcovMfK/LTP_DIR_B
move_mount03.c:100: TPASS: access(DIRB "/A", F_OK) passed
move_mount03.c:102: TINFO: Umounting /tmp/LTP_movcovMfK/LTP_DIR_B
move_mount03.c:103: TINFO: Umounting /tmp/LTP_movcovMfK/LTP_DIR_A
move_mount03.c:77: TBROK: mkdir(LTP_DIR_A, 0777) failed: EEXIST (17)

NOTE: you can speed up the review process, if you check list of common errors
before sending patch to ML 
https://github.com/linux-test-project/ltp/wiki/Maintainer-Patch-Review-Checklist#how-to-find-clear-errors

make check-move_mount03
CHECK testcases/kernel/syscalls/move_mount/move_mount03.c
move_mount03.c:92:9: error: undefined identifier 'MOVE_MOUNT_BENEATH'

I see false positive from checkpatch.pl, it would be interesting to check if it
can be fixed.

> diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
> new file mode 100644
> index 000000000..dadb19178
> --- /dev/null
> +++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Christian Brauner <brauner@kernel.org>
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Verify allow to mount beneath top mount base following commit:
This will be very badly formatted in docs.
> + * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
> + * Author: Christian Brauner <brauner@kernel.org>
> + * Date:   Wed May 3 13:18:42 2023 +0200
> + *     fs: allow to mount beneath top mount
> + *
> + * Above commit has heavily commented but i found following commit
> + * contain simple summary of this feature for easy understanding:
> + *
> + * commit c0a572d9d32fe1e95672f24e860776dba0750a38
> + * Author: Linus Torvalds <torvalds@linux-foundation.org>
> + *       TL;DR:
> + *

Please generate metadata and have look at result. The output below looks ugly.

Either you find a way to preformat it as generated output (put extra space after
* in the comment - equivalent of <pre>... </pre> HTML output) or put it into /*
* */, so that it's not in docs (good for some info useful in C source but more
related to the test implementation so that it does not have to be in generated docs).

> + *         > mount -t ext4 /dev/sda /mnt
> + *           |
> + *           --/mnt    /dev/sda    ext4
> + *
> + *         > mount --beneath -t xfs /dev/sdb /mnt
> + *           |
> + *           --/mnt    /dev/sdb    xfs
> + *             --/mnt  /dev/sda    ext4
> + *
> + *         > umount /mnt
> + *           |
> + *           --/mnt    /dev/sdb    xfs
> + *
> + * So base above scenario design following scenario for LTP check:
> + *
> + *         > mount -t tmpfs /DIRA
> + *           |
> + *           --/DIRA(create A file within DIRA)
> + *
> + *         > mount -t tmpfs /DIRB
> + *           |
> + *           --/DIRA(create B file within DIRB)
> + *
> + *         > move_mount --beneath /DIRA /DIRB
> + *           |
> + *           --/mnt    /DIRA /DIRB
> + *             --/mnt  /DIRB
> + *
> + *         If you check content of /DIRB, you can see file B
> + *
> + *         > umount /DIRB
> + *           |
> + *           --/mnt    /DIRA /DIRB
> + *         Check content of /DIRB, you can see file A exist since
> + *         current /DIRB mount source is already become /DIRA
> + *
> + * More detail can be found in following link:
> + * Link: https://lwn.net/Articles/930591/
> + * Link: https://github.com/brauner/move-mount-beneath
Link is bogus (we use it only as tag in the git commit message, not in docs).

 * See also:
 * https://lwn.net/Articles/930591/
 * https://github.com/brauner/move-mount-beneath
 */

> + */
> +
> +#include <stdio.h>
> +
> +#include "tst_test.h"
> +#include "lapi/fsmount.h"
> +#include "lapi/sched.h"
> +
> +#define DIRA "LTP_DIR_A"
> +#define DIRB "LTP_DIR_B"
> +
> +static void run(void)
> +{
> +	int fda, fdb;
> +
> +	SAFE_MKDIR(DIRA, 0777);
> +	SAFE_MKDIR(DIRB, 0777);
> +	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
> +	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
> +	SAFE_TOUCH(DIRA "/A", 0, NULL);
> +	SAFE_TOUCH(DIRB "/B", 0, NULL);
Maybe whole code in above could be in setup() function, it can be created only
once even we run test more times with -iN, right? Setup function is for test
speedup (imagine somebody run test with -i10000, why to create files all the
time?
> +
> +	TEST(fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE));
> +
> +	if (fda == -1) {
> +		tst_res(TFAIL | TTERRNO, "open_tree() failed");
> +		return;
> +	}
Maybe this should also be in the setup function. also not using TEST() for it?


fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
if (fda > 0)
	tst_brk(TBROK | TERRNO, "open_tree() failed");

> +
> +	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
> +	TST_EXP_PASS(move_mount(fda, "", fdb, "",
> +				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
> +				MOVE_MOUNT_T_EMPTY_PATH));

I wonder what happen if any of the following SAFE_*() functions fail.
There will be unrelated errors and left mount directories. IMHO at least
SAFE_UMOUNT() should be in cleanup functions and guarded by flags (e.g. not
trying to umount() when nothing was mounted because tst_brk() earlier).

Kind regards,
Petr

> +	SAFE_CLOSE(fda);
> +	SAFE_CLOSE(fdb);
> +
> +	TST_EXP_PASS(access(DIRB "/B", F_OK));
> +	SAFE_UMOUNT(DIRB);
> +	TST_EXP_PASS(access(DIRB "/A", F_OK));
> +
> +	SAFE_UMOUNT(DIRB);
> +	SAFE_UMOUNT(DIRA);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.needs_root = 1,
> +	.min_kver = "6.5.0",
> +	.needs_tmpdir = 1,
> +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] move_mount03: check allow to mount beneath top mount

[Wei Gao via ltp]

Hi Petr
Very strange I have not found this email in my mutt, so I have to reply it in my outlook, sorry for inconvenience.
Regards
Gao Wei

I suppose there was no v1, right?
[GW] V1:  https://patchwork.ozlabs.org/project/ltp/patch/20230913101542.18550-1-wegao@suse.com/

NOTE: you can speed up the review process, if you check list of common errors before sending patch to ML https://github.com/linux-test-project/ltp/wiki/Maintainer-Patch-Review-Checklist#how-to-find-clear-errors
[GW]: patch v2 send with following check: make check, m32, run with -i3

I see false positive from checkpatch.pl, it would be interesting to check if it can be fixed.
[GW]: v1 has no this issue, I wrongly remove some change from v1.

> +static void run(void)
> +{
> +	int fda, fdb;
> +
> +	SAFE_MKDIR(DIRA, 0777);
> +	SAFE_MKDIR(DIRB, 0777);
> +	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
> +	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
> +	SAFE_TOUCH(DIRA "/A", 0, NULL);
> +	SAFE_TOUCH(DIRB "/B", 0, NULL);
Maybe whole code in above could be in setup() function, it can be created only once even we run test more times with -iN, right? Setup function is for test speedup (imagine somebody run test with -i10000, why to create files all the time?
[GW]: MKDIR should put into setup otherwise issue will happen, but mount & touch should keep within run function since move_mount will change mount point later and we need rebuild mount env again when it rerun.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3] move_mount03: check allow to mount beneath top mount

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
---
 include/lapi/fsmount.h                        |   4 +
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/move_mount/.gitignore     |   1 +
 .../kernel/syscalls/move_mount/move_mount03.c | 128 ++++++++++++++++++
 4 files changed, 134 insertions(+)
 create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c

diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
index 07eb42ffa..216e966c7 100644
--- a/include/lapi/fsmount.h
+++ b/include/lapi/fsmount.h
@@ -115,6 +115,10 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
 }
 #endif /* HAVE_MOUNT_SETATTR */
 
+#ifndef MOVE_MOUNT_BENEATH
+#define MOVE_MOUNT_BENEATH 		0x00000200
+#endif /* MOVE_MOUNT_BENEATH */
+
 /*
  * New headers added in kernel after 5.2 release, create them for old userspace.
 */
diff --git a/runtest/syscalls b/runtest/syscalls
index b1125dd75..04b758fd9 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
 
 move_mount01 move_mount01
 move_mount02 move_mount02
+move_mount03 move_mount03
 
 move_pages01 move_pages01
 move_pages02 move_pages02
diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
index 83ae40145..ddfe10128 100644
--- a/testcases/kernel/syscalls/move_mount/.gitignore
+++ b/testcases/kernel/syscalls/move_mount/.gitignore
@@ -1,2 +1,3 @@
 /move_mount01
 /move_mount02
+/move_mount03
diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
new file mode 100644
index 000000000..fff95c50b
--- /dev/null
+++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Christian Brauner <brauner@kernel.org>
+ * Copyright (c) 2023 Wei Gao <wegao@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Test allow to mount beneath top mount feature
+ */
+
+/*
+ * Test create for following commit:
+ * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
+ * Author: Christian Brauner <brauner@kernel.org>
+ * Date:   Wed May 3 13:18:42 2023 +0200
+ *     fs: allow to mount beneath top mount
+ *
+ * Above commit has heavily commented but i found following commit
+ * contain simple summary of this feature for easy understanding:
+ *
+ * commit c0a572d9d32fe1e95672f24e860776dba0750a38
+ * Author: Linus Torvalds <torvalds@linux-foundation.org>
+ *       TL;DR:
+ *
+ *         > mount -t ext4 /dev/sda /mnt
+ *           |
+ *           --/mnt    /dev/sda    ext4
+ *
+ *         > mount --beneath -t xfs /dev/sdb /mnt
+ *           |
+ *           --/mnt    /dev/sdb    xfs
+ *             --/mnt  /dev/sda    ext4
+ *
+ *         > umount /mnt
+ *           |
+ *           --/mnt    /dev/sdb    xfs
+ *
+ * So base above scenario design following scenario for LTP check:
+ *
+ *         > mount -t tmpfs /DIRA
+ *           |
+ *           --/DIRA(create A file within DIRA)
+ *
+ *         > mount -t tmpfs /DIRB
+ *           |
+ *           --/DIRA(create B file within DIRB)
+ *
+ *         > move_mount --beneath /DIRA /DIRB
+ *           |
+ *           --/mnt    /DIRA /DIRB
+ *             --/mnt  /DIRB
+ *
+ *         If you check content of /DIRB, you can see file B
+ *
+ *         > umount /DIRB
+ *           |
+ *           --/mnt    /DIRA /DIRB
+ *         Check content of /DIRB, you can see file A exist since
+ *         current /DIRB mount source is already become /DIRA
+ *
+ * See also:
+ * https://lwn.net/Articles/930591/
+ * https://github.com/brauner/move-mount-beneath
+ */
+
+#include <stdio.h>
+
+#include "tst_test.h"
+#include "lapi/fsmount.h"
+#include "lapi/sched.h"
+
+#define DIRA "LTP_DIR_A"
+#define DIRB "LTP_DIR_B"
+
+static void run(void)
+{
+	int fda, fdb;
+
+	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
+	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
+	SAFE_TOUCH(DIRA "/A", 0, NULL);
+	SAFE_TOUCH(DIRB "/B", 0, NULL);
+
+	/* TEST(fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE)); */
+	fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
+	if (fda == -1)
+		tst_brk(TBROK | TERRNO, "open_tree() failed");
+
+	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
+	TST_EXP_PASS(move_mount(fda, "", fdb, "",
+				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
+				MOVE_MOUNT_T_EMPTY_PATH));
+	SAFE_CLOSE(fda);
+	SAFE_CLOSE(fdb);
+
+	TST_EXP_PASS(access(DIRB "/B", F_OK));
+	SAFE_UMOUNT(DIRB);
+	TST_EXP_PASS(access(DIRB "/A", F_OK));
+
+	SAFE_UMOUNT(DIRB);
+	SAFE_UMOUNT(DIRA);
+}
+
+static void setup(void)
+{
+	SAFE_MKDIR(DIRA, 0777);
+	SAFE_MKDIR(DIRB, 0777);
+}
+
+static void cleanup(void)
+{
+	if (tst_is_mounted_at_tmpdir(DIRA))
+		SAFE_UMOUNT(DIRA);
+
+	if (tst_is_mounted_at_tmpdir(DIRB))
+		SAFE_UMOUNT(DIRB);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.needs_root = 1,
+	.min_kver = "6.5.0",
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3] move_mount03: check allow to mount beneath top mount

[Martin Doucha]

Hi,
some comments below.

On 28. 12. 23 3:55, Wei Gao via ltp wrote:
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>   include/lapi/fsmount.h                        |   4 +
>   runtest/syscalls                              |   1 +
>   .../kernel/syscalls/move_mount/.gitignore     |   1 +
>   .../kernel/syscalls/move_mount/move_mount03.c | 128 ++++++++++++++++++
>   4 files changed, 134 insertions(+)
>   create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c
> 
> diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
> index 07eb42ffa..216e966c7 100644
> --- a/include/lapi/fsmount.h
> +++ b/include/lapi/fsmount.h
> @@ -115,6 +115,10 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
>   }
>   #endif /* HAVE_MOUNT_SETATTR */
>   
> +#ifndef MOVE_MOUNT_BENEATH
> +#define MOVE_MOUNT_BENEATH 		0x00000200
> +#endif /* MOVE_MOUNT_BENEATH */
> +
>   /*
>    * New headers added in kernel after 5.2 release, create them for old userspace.
>   */
> diff --git a/runtest/syscalls b/runtest/syscalls
> index b1125dd75..04b758fd9 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
>   
>   move_mount01 move_mount01
>   move_mount02 move_mount02
> +move_mount03 move_mount03
>   
>   move_pages01 move_pages01
>   move_pages02 move_pages02
> diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
> index 83ae40145..ddfe10128 100644
> --- a/testcases/kernel/syscalls/move_mount/.gitignore
> +++ b/testcases/kernel/syscalls/move_mount/.gitignore
> @@ -1,2 +1,3 @@
>   /move_mount01
>   /move_mount02
> +/move_mount03
> diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
> new file mode 100644
> index 000000000..fff95c50b
> --- /dev/null
> +++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Christian Brauner <brauner@kernel.org>
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Test allow to mount beneath top mount feature
> + */
> +
> +/*
> + * Test create for following commit:
> + * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
> + * Author: Christian Brauner <brauner@kernel.org>
> + * Date:   Wed May 3 13:18:42 2023 +0200
> + *     fs: allow to mount beneath top mount
> + *
> + * Above commit has heavily commented but i found following commit
> + * contain simple summary of this feature for easy understanding:
> + *
> + * commit c0a572d9d32fe1e95672f24e860776dba0750a38
> + * Author: Linus Torvalds <torvalds@linux-foundation.org>
> + *       TL;DR:
> + *
> + *         > mount -t ext4 /dev/sda /mnt
> + *           |
> + *           --/mnt    /dev/sda    ext4
> + *
> + *         > mount --beneath -t xfs /dev/sdb /mnt
> + *           |
> + *           --/mnt    /dev/sdb    xfs
> + *             --/mnt  /dev/sda    ext4
> + *
> + *         > umount /mnt
> + *           |
> + *           --/mnt    /dev/sdb    xfs
> + *
> + * So base above scenario design following scenario for LTP check:
> + *
> + *         > mount -t tmpfs /DIRA
> + *           |
> + *           --/DIRA(create A file within DIRA)
> + *
> + *         > mount -t tmpfs /DIRB
> + *           |
> + *           --/DIRA(create B file within DIRB)
> + *
> + *         > move_mount --beneath /DIRA /DIRB
> + *           |
> + *           --/mnt    /DIRA /DIRB
> + *             --/mnt  /DIRB
> + *
> + *         If you check content of /DIRB, you can see file B
> + *
> + *         > umount /DIRB
> + *           |
> + *           --/mnt    /DIRA /DIRB
> + *         Check content of /DIRB, you can see file A exist since
> + *         current /DIRB mount source is already become /DIRA
> + *
> + * See also:
> + * https://lwn.net/Articles/930591/
> + * https://github.com/brauner/move-mount-beneath
> + */
> +
> +#include <stdio.h>
> +
> +#include "tst_test.h"
> +#include "lapi/fsmount.h"
> +#include "lapi/sched.h"
> +
> +#define DIRA "LTP_DIR_A"
> +#define DIRB "LTP_DIR_B"
> +
> +static void run(void)
> +{
> +	int fda, fdb;
> +
> +	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
> +	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
> +	SAFE_TOUCH(DIRA "/A", 0, NULL);
> +	SAFE_TOUCH(DIRB "/B", 0, NULL);
> +
> +	/* TEST(fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE)); */

Is the comment needed?

> +	fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
> +	if (fda == -1)
> +		tst_brk(TBROK | TERRNO, "open_tree() failed");
> +
> +	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
> +	TST_EXP_PASS(move_mount(fda, "", fdb, "",
> +				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
> +				MOVE_MOUNT_T_EMPTY_PATH));
> +	SAFE_CLOSE(fda);
> +	SAFE_CLOSE(fdb);
> +
> +	TST_EXP_PASS(access(DIRB "/B", F_OK));

It'd also make sense the check here that file A still exists in DIRA but 
not in DIRB.

> +	SAFE_UMOUNT(DIRB);
> +	TST_EXP_PASS(access(DIRB "/A", F_OK));
> +
> +	SAFE_UMOUNT(DIRB);
> +	SAFE_UMOUNT(DIRA);
> +}
> +
> +static void setup(void)
> +{
> +	SAFE_MKDIR(DIRA, 0777);
> +	SAFE_MKDIR(DIRB, 0777);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (tst_is_mounted_at_tmpdir(DIRA))
> +		SAFE_UMOUNT(DIRA);
> +
> +	if (tst_is_mounted_at_tmpdir(DIRB))
> +		SAFE_UMOUNT(DIRB);

I believe that DIRB may need to be unmounted twice here. Also please 
close fda and fdb here if they're still open.

> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.needs_root = 1,
> +	.min_kver = "6.5.0",
> +	.needs_tmpdir = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3] move_mount03: check allow to mount beneath top mount

[Petr Vorel]

> Hi,
> some comments below.

> On 28. 12. 23 3:55, Wei Gao via ltp wrote:
> > Signed-off-by: Wei Gao <wegao@suse.com>
> > ---
> >   include/lapi/fsmount.h                        |   4 +
> >   runtest/syscalls                              |   1 +
> >   .../kernel/syscalls/move_mount/.gitignore     |   1 +
> >   .../kernel/syscalls/move_mount/move_mount03.c | 128 ++++++++++++++++++
> >   4 files changed, 134 insertions(+)
> >   create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c

> > diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
> > index 07eb42ffa..216e966c7 100644
> > --- a/include/lapi/fsmount.h
> > +++ b/include/lapi/fsmount.h
> > @@ -115,6 +115,10 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
> >   }
> >   #endif /* HAVE_MOUNT_SETATTR */
> > +#ifndef MOVE_MOUNT_BENEATH
> > +#define MOVE_MOUNT_BENEATH 		0x00000200
> > +#endif /* MOVE_MOUNT_BENEATH */
> > +
> >   /*
> >    * New headers added in kernel after 5.2 release, create them for old userspace.
> >   */
> > diff --git a/runtest/syscalls b/runtest/syscalls
> > index b1125dd75..04b758fd9 100644
> > --- a/runtest/syscalls
> > +++ b/runtest/syscalls
> > @@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
> >   move_mount01 move_mount01
> >   move_mount02 move_mount02
> > +move_mount03 move_mount03
> >   move_pages01 move_pages01
> >   move_pages02 move_pages02
> > diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
> > index 83ae40145..ddfe10128 100644
> > --- a/testcases/kernel/syscalls/move_mount/.gitignore
> > +++ b/testcases/kernel/syscalls/move_mount/.gitignore
> > @@ -1,2 +1,3 @@
> >   /move_mount01
> >   /move_mount02
> > +/move_mount03
> > diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
> > new file mode 100644
> > index 000000000..fff95c50b
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
> > @@ -0,0 +1,128 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2023 Christian Brauner <brauner@kernel.org>
> > + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> > + */
> > +
> > +/*\
> > + * [Description]
> > + *
> > + * Test allow to mount beneath top mount feature
> > + */
> > +
> > +/*
> > + * Test create for following commit:
> > + * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
> > + * Author: Christian Brauner <brauner@kernel.org>
> > + * Date:   Wed May 3 13:18:42 2023 +0200
> > + *     fs: allow to mount beneath top mount
> > + *
> > + * Above commit has heavily commented but i found following commit
> > + * contain simple summary of this feature for easy understanding:
> > + *
> > + * commit c0a572d9d32fe1e95672f24e860776dba0750a38
> > + * Author: Linus Torvalds <torvalds@linux-foundation.org>
> > + *       TL;DR:
> > + *
> > + *         > mount -t ext4 /dev/sda /mnt
> > + *           |
> > + *           --/mnt    /dev/sda    ext4
> > + *
> > + *         > mount --beneath -t xfs /dev/sdb /mnt
> > + *           |
> > + *           --/mnt    /dev/sdb    xfs
> > + *             --/mnt  /dev/sda    ext4
> > + *
> > + *         > umount /mnt
> > + *           |
> > + *           --/mnt    /dev/sdb    xfs
> > + *
> > + * So base above scenario design following scenario for LTP check:
> > + *
> > + *         > mount -t tmpfs /DIRA
> > + *           |
> > + *           --/DIRA(create A file within DIRA)
> > + *
> > + *         > mount -t tmpfs /DIRB
> > + *           |
> > + *           --/DIRA(create B file within DIRB)
> > + *
> > + *         > move_mount --beneath /DIRA /DIRB
> > + *           |
> > + *           --/mnt    /DIRA /DIRB
> > + *             --/mnt  /DIRB
> > + *
> > + *         If you check content of /DIRB, you can see file B
> > + *
> > + *         > umount /DIRB
> > + *           |
> > + *           --/mnt    /DIRA /DIRB
> > + *         Check content of /DIRB, you can see file A exist since
> > + *         current /DIRB mount source is already become /DIRA
> > + *
> > + * See also:
> > + * https://lwn.net/Articles/930591/
> > + * https://github.com/brauner/move-mount-beneath
> > + */
> > +
> > +#include <stdio.h>
> > +
> > +#include "tst_test.h"
> > +#include "lapi/fsmount.h"
> > +#include "lapi/sched.h"
> > +
> > +#define DIRA "LTP_DIR_A"
> > +#define DIRB "LTP_DIR_B"
> > +
> > +static void run(void)
> > +{
> > +	int fda, fdb;
> > +
> > +	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
> > +	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
> > +	SAFE_TOUCH(DIRA "/A", 0, NULL);
> > +	SAFE_TOUCH(DIRB "/B", 0, NULL);
> > +
> > +	/* TEST(fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE)); */

> Is the comment needed?

+1

> > +	fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
> > +	if (fda == -1)
> > +		tst_brk(TBROK | TERRNO, "open_tree() failed");
> > +
> > +	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
> > +	TST_EXP_PASS(move_mount(fda, "", fdb, "",
> > +				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
> > +				MOVE_MOUNT_T_EMPTY_PATH));
> > +	SAFE_CLOSE(fda);
> > +	SAFE_CLOSE(fdb);
> > +
> > +	TST_EXP_PASS(access(DIRB "/B", F_OK));

> It'd also make sense the check here that file A still exists in DIRA but not
> in DIRB.

> > +	SAFE_UMOUNT(DIRB);
> > +	TST_EXP_PASS(access(DIRB "/A", F_OK));
> > +
> > +	SAFE_UMOUNT(DIRB);
> > +	SAFE_UMOUNT(DIRA);
> > +}
> > +
> > +static void setup(void)
> > +{
> > +	SAFE_MKDIR(DIRA, 0777);
> > +	SAFE_MKDIR(DIRB, 0777);
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > +	if (tst_is_mounted_at_tmpdir(DIRA))
> > +		SAFE_UMOUNT(DIRA);
> > +
> > +	if (tst_is_mounted_at_tmpdir(DIRB))
> > +		SAFE_UMOUNT(DIRB);

> I believe that DIRB may need to be unmounted twice here. Also please close
> fda and fdb here if they're still open.

What would do the second mount? move_mount() ?

There are SAFE_UMOUNT() in the end of the testing function, that's why usually
even these 2 SAFE_UMOUNT() aren't needed (but obviously we need to add them if
SAFE_TOUCH() or something quits testing early):

move_mount03.c:92: TPASS: move_mount(fda, "", fdb, "", MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH | MOVE_MOUNT_T_EMPTY_PATH) passed
move_mount03.c:98: TPASS: access(DIRB "/B", F_OK) passed
move_mount03.c:99: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_B
move_mount03.c:100: TPASS: access(DIRB "/A", F_OK) passed
move_mount03.c:102: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_B
move_mount03.c:103: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_A
tst_device.c:442: TINFO: No device is mounted at /tmp/LTP_movqeTZGu/LTP_DIR_A
tst_device.c:442: TINFO: No device is mounted at /tmp/LTP_movqeTZGu/LTP_DIR_B

Kind regards,
Petr

> > +}
> > +
> > +static struct tst_test test = {
> > +	.test_all = run,
> > +	.needs_root = 1,
> > +	.min_kver = "6.5.0",
> > +	.needs_tmpdir = 1,
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3] move_mount03: check allow to mount beneath top mount

[Martin Doucha]

On 20. 03. 24 8:43, Petr Vorel wrote:
>> On 28. 12. 23 3:55, Wei Gao via ltp wrote:
>>> +static void cleanup(void)
>>> +{
>>> +	if (tst_is_mounted_at_tmpdir(DIRA))
>>> +		SAFE_UMOUNT(DIRA);
>>> +
>>> +	if (tst_is_mounted_at_tmpdir(DIRB))
>>> +		SAFE_UMOUNT(DIRB);
> 
>> I believe that DIRB may need to be unmounted twice here. Also please close
>> fda and fdb here if they're still open.
> 
> What would do the second mount? move_mount() ?
> 
> There are SAFE_UMOUNT() in the end of the testing function, that's why usually
> even these 2 SAFE_UMOUNT() aren't needed (but obviously we need to add them if
> SAFE_TOUCH() or something quits testing early):
> 
> move_mount03.c:92: TPASS: move_mount(fda, "", fdb, "", MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH | MOVE_MOUNT_T_EMPTY_PATH) passed
> move_mount03.c:98: TPASS: access(DIRB "/B", F_OK) passed
> move_mount03.c:99: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_B
> move_mount03.c:100: TPASS: access(DIRB "/A", F_OK) passed
> move_mount03.c:102: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_B
> move_mount03.c:103: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_A
> tst_device.c:442: TINFO: No device is mounted at /tmp/LTP_movqeTZGu/LTP_DIR_A
> tst_device.c:442: TINFO: No device is mounted at /tmp/LTP_movqeTZGu/LTP_DIR_B

Yes, open_tree() was called with OPEN_TREE_CLONE so after move_mount(), 
the mounts look like this:
- tmpfs[1] at DIRA
- tmpfs[1] at DIRB
- tmpfs[2] at DIRB

If either SAFE_CLOSE() immediately after move_mount() fails, cleanup() 
will unmount tmpfs[1] from DIRA and tmpfs[2] from DIRB but leave 
tmpfs[1] mounted at DIRB.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3] move_mount03: check allow to mount beneath top mount

[Petr Vorel]

> On 20. 03. 24 8:43, Petr Vorel wrote:
> > > On 28. 12. 23 3:55, Wei Gao via ltp wrote:
> > > > +static void cleanup(void)
> > > > +{
> > > > +	if (tst_is_mounted_at_tmpdir(DIRA))
> > > > +		SAFE_UMOUNT(DIRA);
> > > > +
> > > > +	if (tst_is_mounted_at_tmpdir(DIRB))
> > > > +		SAFE_UMOUNT(DIRB);

> > > I believe that DIRB may need to be unmounted twice here. Also please close
> > > fda and fdb here if they're still open.

> > What would do the second mount? move_mount() ?

> > There are SAFE_UMOUNT() in the end of the testing function, that's why usually
> > even these 2 SAFE_UMOUNT() aren't needed (but obviously we need to add them if
> > SAFE_TOUCH() or something quits testing early):

> > move_mount03.c:92: TPASS: move_mount(fda, "", fdb, "", MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH | MOVE_MOUNT_T_EMPTY_PATH) passed
> > move_mount03.c:98: TPASS: access(DIRB "/B", F_OK) passed
> > move_mount03.c:99: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_B
> > move_mount03.c:100: TPASS: access(DIRB "/A", F_OK) passed
> > move_mount03.c:102: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_B
> > move_mount03.c:103: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_A
> > tst_device.c:442: TINFO: No device is mounted at /tmp/LTP_movqeTZGu/LTP_DIR_A
> > tst_device.c:442: TINFO: No device is mounted at /tmp/LTP_movqeTZGu/LTP_DIR_B

> Yes, open_tree() was called with OPEN_TREE_CLONE so after move_mount(), the
> mounts look like this:
> - tmpfs[1] at DIRA
> - tmpfs[1] at DIRB
> - tmpfs[2] at DIRB

> If either SAFE_CLOSE() immediately after move_mount() fails, cleanup() will
> unmount tmpfs[1] from DIRA and tmpfs[2] from DIRB but leave tmpfs[1] mounted
> at DIRB.

@Martin Thanks!
@Wai Please send another version.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3] move_mount03: check allow to mount beneath top mount

[Petr Vorel]

> On 20. 03. 24 8:43, Petr Vorel wrote:
> > > On 28. 12. 23 3:55, Wei Gao via ltp wrote:
> > > > +static void cleanup(void)
> > > > +{
> > > > +	if (tst_is_mounted_at_tmpdir(DIRA))
> > > > +		SAFE_UMOUNT(DIRA);
> > > > +
> > > > +	if (tst_is_mounted_at_tmpdir(DIRB))
> > > > +		SAFE_UMOUNT(DIRB);

> > > I believe that DIRB may need to be unmounted twice here. Also please close
> > > fda and fdb here if they're still open.

> > What would do the second mount? move_mount() ?

> > There are SAFE_UMOUNT() in the end of the testing function, that's why usually
> > even these 2 SAFE_UMOUNT() aren't needed (but obviously we need to add them if
> > SAFE_TOUCH() or something quits testing early):

> > move_mount03.c:92: TPASS: move_mount(fda, "", fdb, "", MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH | MOVE_MOUNT_T_EMPTY_PATH) passed
> > move_mount03.c:98: TPASS: access(DIRB "/B", F_OK) passed
> > move_mount03.c:99: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_B
> > move_mount03.c:100: TPASS: access(DIRB "/A", F_OK) passed
> > move_mount03.c:102: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_B
> > move_mount03.c:103: TINFO: Umounting /tmp/LTP_movqeTZGu/LTP_DIR_A
> > tst_device.c:442: TINFO: No device is mounted at /tmp/LTP_movqeTZGu/LTP_DIR_A
> > tst_device.c:442: TINFO: No device is mounted at /tmp/LTP_movqeTZGu/LTP_DIR_B

> Yes, open_tree() was called with OPEN_TREE_CLONE so after move_mount(), the
> mounts look like this:
> - tmpfs[1] at DIRA
> - tmpfs[1] at DIRB
> - tmpfs[2] at DIRB

> If either SAFE_CLOSE() immediately after move_mount() fails, cleanup() will
> unmount tmpfs[1] from DIRA and tmpfs[2] from DIRB but leave tmpfs[1] mounted
> at DIRB.

Correct. Thanks!

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4] move_mount03: check allow to mount beneath top mount

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
---
 include/lapi/fsmount.h                        |   4 +
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/move_mount/.gitignore     |   1 +
 .../kernel/syscalls/move_mount/move_mount03.c | 137 ++++++++++++++++++
 4 files changed, 143 insertions(+)
 create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c

diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
index 07eb42ffa..216e966c7 100644
--- a/include/lapi/fsmount.h
+++ b/include/lapi/fsmount.h
@@ -115,6 +115,10 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
 }
 #endif /* HAVE_MOUNT_SETATTR */
 
+#ifndef MOVE_MOUNT_BENEATH
+#define MOVE_MOUNT_BENEATH 		0x00000200
+#endif /* MOVE_MOUNT_BENEATH */
+
 /*
  * New headers added in kernel after 5.2 release, create them for old userspace.
 */
diff --git a/runtest/syscalls b/runtest/syscalls
index b1125dd75..04b758fd9 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
 
 move_mount01 move_mount01
 move_mount02 move_mount02
+move_mount03 move_mount03
 
 move_pages01 move_pages01
 move_pages02 move_pages02
diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
index 83ae40145..ddfe10128 100644
--- a/testcases/kernel/syscalls/move_mount/.gitignore
+++ b/testcases/kernel/syscalls/move_mount/.gitignore
@@ -1,2 +1,3 @@
 /move_mount01
 /move_mount02
+/move_mount03
diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
new file mode 100644
index 000000000..dfdad080e
--- /dev/null
+++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Christian Brauner <brauner@kernel.org>
+ * Copyright (c) 2023 Wei Gao <wegao@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Test allow to mount beneath top mount feature
+ */
+
+/*
+ * Test create for following commit:
+ * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
+ * Author: Christian Brauner <brauner@kernel.org>
+ * Date:   Wed May 3 13:18:42 2023 +0200
+ *     fs: allow to mount beneath top mount
+ *
+ * Above commit has heavily commented but i found following commit
+ * contain simple summary of this feature for easy understanding:
+ *
+ * commit c0a572d9d32fe1e95672f24e860776dba0750a38
+ * Author: Linus Torvalds <torvalds@linux-foundation.org>
+ *       TL;DR:
+ *
+ *         > mount -t ext4 /dev/sda /mnt
+ *           |
+ *           --/mnt    /dev/sda    ext4
+ *
+ *         > mount --beneath -t xfs /dev/sdb /mnt
+ *           |
+ *           --/mnt    /dev/sdb    xfs
+ *             --/mnt  /dev/sda    ext4
+ *
+ *         > umount /mnt
+ *           |
+ *           --/mnt    /dev/sdb    xfs
+ *
+ * So base above scenario design following scenario for LTP check:
+ *
+ *         > mount -t tmpfs /DIRA
+ *           |
+ *           --/DIRA(create A file within DIRA)
+ *
+ *         > mount -t tmpfs /DIRB
+ *           |
+ *           --/DIRA(create B file within DIRB)
+ *
+ *         > move_mount --beneath /DIRA /DIRB
+ *           |
+ *           --/mnt    /DIRA /DIRB
+ *             --/mnt  /DIRB
+ *
+ *         If you check content of /DIRB, you can see file B
+ *
+ *         > umount /DIRB
+ *           |
+ *           --/mnt    /DIRA /DIRB
+ *         Check content of /DIRB, you can see file A exist since
+ *         current /DIRB mount source is already become /DIRA
+ *
+ * See also:
+ * https://lwn.net/Articles/930591/
+ * https://github.com/brauner/move-mount-beneath
+ */
+
+#include <stdio.h>
+
+#include "tst_test.h"
+#include "lapi/fsmount.h"
+#include "lapi/sched.h"
+
+#define DIRA "LTP_DIR_A"
+#define DIRB "LTP_DIR_B"
+
+static int fda = -1, fdb = -1;
+
+static void run(void)
+{
+	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
+	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
+	SAFE_TOUCH(DIRA "/A", 0, NULL);
+	SAFE_TOUCH(DIRB "/B", 0, NULL);
+
+	fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
+	if (fda == -1)
+		tst_brk(TBROK | TERRNO, "open_tree() failed");
+
+	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
+	TST_EXP_PASS(move_mount(fda, "", fdb, "",
+				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
+				MOVE_MOUNT_T_EMPTY_PATH));
+	SAFE_CLOSE(fda);
+	SAFE_CLOSE(fdb);
+
+	TST_EXP_PASS(access(DIRB "/B", F_OK));
+	SAFE_UMOUNT(DIRB);
+	TST_EXP_PASS(access(DIRB "/A", F_OK));
+
+	SAFE_UMOUNT(DIRB);
+	SAFE_UMOUNT(DIRA);
+}
+
+static void setup(void)
+{
+	SAFE_MKDIR(DIRA, 0777);
+	SAFE_MKDIR(DIRB, 0777);
+}
+
+static void cleanup(void)
+{
+
+	if (fda >= 0)
+		SAFE_CLOSE(fda);
+
+	if (fdb >= 0)
+		SAFE_CLOSE(fdb);
+
+	if (tst_is_mounted_at_tmpdir(DIRA))
+		SAFE_UMOUNT(DIRA);
+
+	if (tst_is_mounted_at_tmpdir(DIRB))
+		SAFE_UMOUNT(DIRB);
+
+	if (tst_is_mounted_at_tmpdir(DIRB))
+		SAFE_UMOUNT(DIRB);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.needs_root = 1,
+	.min_kver = "6.5.0",
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4] move_mount03: check allow to mount beneath top mount

[Martin Doucha]

Hi,
one note below but this can be merged as is.

Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 22. 03. 24 12:20, Wei Gao via ltp wrote:
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>   include/lapi/fsmount.h                        |   4 +
>   runtest/syscalls                              |   1 +
>   .../kernel/syscalls/move_mount/.gitignore     |   1 +
>   .../kernel/syscalls/move_mount/move_mount03.c | 137 ++++++++++++++++++
>   4 files changed, 143 insertions(+)
>   create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c
> 
> diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
> index 07eb42ffa..216e966c7 100644
> --- a/include/lapi/fsmount.h
> +++ b/include/lapi/fsmount.h
> @@ -115,6 +115,10 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
>   }
>   #endif /* HAVE_MOUNT_SETATTR */
>   
> +#ifndef MOVE_MOUNT_BENEATH
> +#define MOVE_MOUNT_BENEATH 		0x00000200
> +#endif /* MOVE_MOUNT_BENEATH */
> +
>   /*
>    * New headers added in kernel after 5.2 release, create them for old userspace.
>   */
> diff --git a/runtest/syscalls b/runtest/syscalls
> index b1125dd75..04b758fd9 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
>   
>   move_mount01 move_mount01
>   move_mount02 move_mount02
> +move_mount03 move_mount03
>   
>   move_pages01 move_pages01
>   move_pages02 move_pages02
> diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
> index 83ae40145..ddfe10128 100644
> --- a/testcases/kernel/syscalls/move_mount/.gitignore
> +++ b/testcases/kernel/syscalls/move_mount/.gitignore
> @@ -1,2 +1,3 @@
>   /move_mount01
>   /move_mount02
> +/move_mount03
> diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
> new file mode 100644
> index 000000000..dfdad080e
> --- /dev/null
> +++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Christian Brauner <brauner@kernel.org>
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Test allow to mount beneath top mount feature
> + */
> +
> +/*
> + * Test create for following commit:
> + * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
> + * Author: Christian Brauner <brauner@kernel.org>
> + * Date:   Wed May 3 13:18:42 2023 +0200
> + *     fs: allow to mount beneath top mount
> + *
> + * Above commit has heavily commented but i found following commit
> + * contain simple summary of this feature for easy understanding:
> + *
> + * commit c0a572d9d32fe1e95672f24e860776dba0750a38
> + * Author: Linus Torvalds <torvalds@linux-foundation.org>
> + *       TL;DR:
> + *
> + *         > mount -t ext4 /dev/sda /mnt
> + *           |
> + *           --/mnt    /dev/sda    ext4
> + *
> + *         > mount --beneath -t xfs /dev/sdb /mnt
> + *           |
> + *           --/mnt    /dev/sdb    xfs
> + *             --/mnt  /dev/sda    ext4
> + *
> + *         > umount /mnt
> + *           |
> + *           --/mnt    /dev/sdb    xfs
> + *
> + * So base above scenario design following scenario for LTP check:
> + *
> + *         > mount -t tmpfs /DIRA
> + *           |
> + *           --/DIRA(create A file within DIRA)
> + *
> + *         > mount -t tmpfs /DIRB
> + *           |
> + *           --/DIRA(create B file within DIRB)
> + *
> + *         > move_mount --beneath /DIRA /DIRB
> + *           |
> + *           --/mnt    /DIRA /DIRB
> + *             --/mnt  /DIRB
> + *
> + *         If you check content of /DIRB, you can see file B
> + *
> + *         > umount /DIRB
> + *           |
> + *           --/mnt    /DIRA /DIRB
> + *         Check content of /DIRB, you can see file A exist since
> + *         current /DIRB mount source is already become /DIRA
> + *
> + * See also:
> + * https://lwn.net/Articles/930591/
> + * https://github.com/brauner/move-mount-beneath
> + */
> +
> +#include <stdio.h>
> +
> +#include "tst_test.h"
> +#include "lapi/fsmount.h"
> +#include "lapi/sched.h"
> +
> +#define DIRA "LTP_DIR_A"
> +#define DIRB "LTP_DIR_B"
> +
> +static int fda = -1, fdb = -1;
> +
> +static void run(void)
> +{
> +	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
> +	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
> +	SAFE_TOUCH(DIRA "/A", 0, NULL);
> +	SAFE_TOUCH(DIRB "/B", 0, NULL);
> +
> +	fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
> +	if (fda == -1)
> +		tst_brk(TBROK | TERRNO, "open_tree() failed");
> +
> +	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
> +	TST_EXP_PASS(move_mount(fda, "", fdb, "",
> +				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
> +				MOVE_MOUNT_T_EMPTY_PATH));
> +	SAFE_CLOSE(fda);
> +	SAFE_CLOSE(fdb);
> +
> +	TST_EXP_PASS(access(DIRB "/B", F_OK));

That extra check I've asked for in v3 would still be nice here.

> +	SAFE_UMOUNT(DIRB);
> +	TST_EXP_PASS(access(DIRB "/A", F_OK));
> +
> +	SAFE_UMOUNT(DIRB);
> +	SAFE_UMOUNT(DIRA);
> +}
> +
> +static void setup(void)
> +{
> +	SAFE_MKDIR(DIRA, 0777);
> +	SAFE_MKDIR(DIRB, 0777);
> +}
> +
> +static void cleanup(void)
> +{
> +
> +	if (fda >= 0)
> +		SAFE_CLOSE(fda);
> +
> +	if (fdb >= 0)
> +		SAFE_CLOSE(fdb);
> +
> +	if (tst_is_mounted_at_tmpdir(DIRA))
> +		SAFE_UMOUNT(DIRA);
> +
> +	if (tst_is_mounted_at_tmpdir(DIRB))
> +		SAFE_UMOUNT(DIRB);
> +
> +	if (tst_is_mounted_at_tmpdir(DIRB))
> +		SAFE_UMOUNT(DIRB);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.needs_root = 1,
> +	.min_kver = "6.5.0",
> +	.needs_tmpdir = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4] move_mount03: check allow to mount beneath top mount

[Petr Vorel]

Hi Wei, Marting,

> Hi,
> one note below but this can be merged as is.

> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
Thanks!

...
> > diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
> > new file mode 100644

...
> +/*\
> + * [Description]
> + *
> + * Test allow to mount beneath top mount feature
very nit: missing dot.
But more important the docparse docs are very sparse now, when everything is in
the documentation visible only when looking into source code.

I would also mention the commit and the original test in docparse, e.g.
something like:

/*\
 * [Description]
 *
 * Test allow to mount beneath top mount feature added in kernel 6.5:
 * 6ac392815628 ("fs: allow to mount beneath top mount")
 *
 * Test based on:
 * https://github.com/brauner/move-mount-beneath
 *
 * See also:
 * https://lore.kernel.org/all/20230202-fs-move-mount-replace-v4-0-98f3d80d7eaa@kernel.org/
 */

> + */
...
> + * See also:
> + * https://lwn.net/Articles/930591/
I would move these two into docparse as in example above.

This cover letter of v3. Wouldn't it be better to link the final version (v4)
and from lwn.net (as in example above)?
https://lore.kernel.org/all/20230202-fs-move-mount-replace-v4-0-98f3d80d7eaa@kernel.org/

> + * https://github.com/brauner/move-mount-beneath
...
> > +static void run(void)
> > +{
> > +	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
> > +	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
> > +	SAFE_TOUCH(DIRA "/A", 0, NULL);
> > +	SAFE_TOUCH(DIRB "/B", 0, NULL);
> > +
> > +	fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
> > +	if (fda == -1)
> > +		tst_brk(TBROK | TERRNO, "open_tree() failed");
> > +
> > +	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
> > +	TST_EXP_PASS(move_mount(fda, "", fdb, "",
> > +				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
> > +				MOVE_MOUNT_T_EMPTY_PATH));
> > +	SAFE_CLOSE(fda);
> > +	SAFE_CLOSE(fdb);
> > +
> > +	TST_EXP_PASS(access(DIRB "/B", F_OK));

> That extra check I've asked for in v3 would still be nice here.

Would you mind to send v5 with checks Martin suggested?

https://lore.kernel.org/ltp/8798c323-8298-49b1-9950-09f2a7c309cb@suse.cz/

> > +	SAFE_UMOUNT(DIRB);
> > +	TST_EXP_PASS(access(DIRB "/A", F_OK));
> > +
> > +	SAFE_UMOUNT(DIRB);
> > +	SAFE_UMOUNT(DIRA);
> > +}
...
> +static void cleanup(void)
> +{
very nit: remove extra space here:
> +
> +	if (fda >= 0)
> +		SAFE_CLOSE(fda);
> +
> +	if (fdb >= 0)
> +		SAFE_CLOSE(fdb);
....

> +static struct tst_test test = {
> +	.test_all = run,
> +	.needs_root = 1,
> +	.min_kver = "6.5.0",
nit: .min_kver = "6.5", would be enough

Otherwise LGTM.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW briefly looking into the Christian's testing source code, these flags are
not yet covered by LTP:
MOVE_MOUNT_SET_GROUP, MOUNT_ATTR_RDONLY, AT_RECURSIVE

Kind regards,
Petr

> +	.needs_tmpdir = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v5] move_mount03: check allow to mount beneath top mount

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
Reviewed-by: Martin Doucha <mdoucha@suse.cz>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 include/lapi/fsmount.h                        |   4 +
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/move_mount/.gitignore     |   1 +
 .../kernel/syscalls/move_mount/move_mount03.c | 145 ++++++++++++++++++
 4 files changed, 151 insertions(+)
 create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c

diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
index 07eb42ffa..216e966c7 100644
--- a/include/lapi/fsmount.h
+++ b/include/lapi/fsmount.h
@@ -115,6 +115,10 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
 }
 #endif /* HAVE_MOUNT_SETATTR */
 
+#ifndef MOVE_MOUNT_BENEATH
+#define MOVE_MOUNT_BENEATH 		0x00000200
+#endif /* MOVE_MOUNT_BENEATH */
+
 /*
  * New headers added in kernel after 5.2 release, create them for old userspace.
 */
diff --git a/runtest/syscalls b/runtest/syscalls
index b1125dd75..04b758fd9 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01
 
 move_mount01 move_mount01
 move_mount02 move_mount02
+move_mount03 move_mount03
 
 move_pages01 move_pages01
 move_pages02 move_pages02
diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore
index 83ae40145..ddfe10128 100644
--- a/testcases/kernel/syscalls/move_mount/.gitignore
+++ b/testcases/kernel/syscalls/move_mount/.gitignore
@@ -1,2 +1,3 @@
 /move_mount01
 /move_mount02
+/move_mount03
diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
new file mode 100644
index 000000000..ad5c8b9ed
--- /dev/null
+++ b/testcases/kernel/syscalls/move_mount/move_mount03.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Christian Brauner <brauner@kernel.org>
+ * Copyright (c) 2023 Wei Gao <wegao@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Test allow to mount beneath top mount feature added in kernel 6.5:
+ * 6ac392815628 ("fs: allow to mount beneath top mount")
+ *
+ * Test based on:
+ * https://github.com/brauner/move-mount-beneath
+ *
+ * See also:
+ * https://lore.kernel.org/all/20230202-fs-move-mount-replace-v4-0-98f3d80d7eaa@kernel.org/
+ * https://lwn.net/Articles/930591/
+ * https://github.com/brauner/move-mount-beneath
+ */
+
+/*
+ * Test create for following commit:
+ * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b
+ * Author: Christian Brauner <brauner@kernel.org>
+ * Date:   Wed May 3 13:18:42 2023 +0200
+ *     fs: allow to mount beneath top mount
+ *
+ * Above commit has heavily commented but i found following commit
+ * contain simple summary of this feature for easy understanding:
+ *
+ * commit c0a572d9d32fe1e95672f24e860776dba0750a38
+ * Author: Linus Torvalds <torvalds@linux-foundation.org>
+ *       TL;DR:
+ *
+ *         > mount -t ext4 /dev/sda /mnt
+ *           |
+ *           --/mnt    /dev/sda    ext4
+ *
+ *         > mount --beneath -t xfs /dev/sdb /mnt
+ *           |
+ *           --/mnt    /dev/sdb    xfs
+ *             --/mnt  /dev/sda    ext4
+ *
+ *         > umount /mnt
+ *           |
+ *           --/mnt    /dev/sdb    xfs
+ *
+ * So base above scenario design following scenario for LTP check:
+ *
+ *         > mount -t tmpfs /DIRA
+ *           |
+ *           --/DIRA(create A file within DIRA)
+ *
+ *         > mount -t tmpfs /DIRB
+ *           |
+ *           --/DIRA(create B file within DIRB)
+ *
+ *         > move_mount --beneath /DIRA /DIRB
+ *           |
+ *           --/mnt    /DIRA /DIRB
+ *             --/mnt  /DIRB
+ *
+ *         If you check content of /DIRB, you can see file B
+ *
+ *         > umount /DIRB
+ *           |
+ *           --/mnt    /DIRA /DIRB
+ *         Check content of /DIRB, you can see file A exist since
+ *         current /DIRB mount source is already become /DIRA
+ *
+ */
+
+#include <stdio.h>
+
+#include "tst_test.h"
+#include "lapi/fsmount.h"
+#include "lapi/sched.h"
+
+#define DIRA "LTP_DIR_A"
+#define DIRB "LTP_DIR_B"
+
+static int fda = -1, fdb = -1;
+
+static void run(void)
+{
+	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
+	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
+	SAFE_TOUCH(DIRA "/A", 0, NULL);
+	SAFE_TOUCH(DIRB "/B", 0, NULL);
+
+	fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
+	if (fda == -1)
+		tst_brk(TBROK | TERRNO, "open_tree() failed");
+
+	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
+	TST_EXP_PASS(move_mount(fda, "", fdb, "",
+				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
+				MOVE_MOUNT_T_EMPTY_PATH));
+	SAFE_CLOSE(fda);
+	SAFE_CLOSE(fdb);
+
+	TST_EXP_PASS(access(DIRB "/B", F_OK));
+	TST_EXP_PASS(access(DIRA "/A", F_OK));
+	TST_EXP_FAIL(access(DIRB "/A", F_OK), ENOENT, "A should not exist");
+
+	SAFE_UMOUNT(DIRB);
+	TST_EXP_PASS(access(DIRB "/A", F_OK));
+
+	SAFE_UMOUNT(DIRB);
+	SAFE_UMOUNT(DIRA);
+}
+
+static void setup(void)
+{
+	SAFE_MKDIR(DIRA, 0777);
+	SAFE_MKDIR(DIRB, 0777);
+}
+
+static void cleanup(void)
+{
+	if (fda >= 0)
+		SAFE_CLOSE(fda);
+
+	if (fdb >= 0)
+		SAFE_CLOSE(fdb);
+
+	if (tst_is_mounted_at_tmpdir(DIRA))
+		SAFE_UMOUNT(DIRA);
+
+	if (tst_is_mounted_at_tmpdir(DIRB))
+		SAFE_UMOUNT(DIRB);
+
+	if (tst_is_mounted_at_tmpdir(DIRB))
+		SAFE_UMOUNT(DIRB);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.needs_root = 1,
+	.min_kver = "6.5",
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v5] move_mount03: check allow to mount beneath top mount

[Petr Vorel]

Hi Wei,

merged with adding 6ac392815628f linux-git tag and formatting changes.
Thank you!

Kind regards,
Petr

+++ include/lapi/fsmount.h
@@ -114,14 +114,14 @@ static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned i
 }
 #endif /* HAVE_MOUNT_SETATTR */
 
-#ifndef MOVE_MOUNT_BENEATH
-#define MOVE_MOUNT_BENEATH 		0x00000200
-#endif /* MOVE_MOUNT_BENEATH */
-
 /*
  * New headers added in kernel after 5.2 release, create them for old userspace.
 */
 
+#ifndef MOVE_MOUNT_BENEATH
+# define MOVE_MOUNT_BENEATH 		0x00000200
+#endif
+
 #ifndef OPEN_TREE_CLONE
 
 /*
diff --git testcases/kernel/syscalls/move_mount/move_mount03.c testcases/kernel/syscalls/move_mount/move_mount03.c
index ad5c8b9ed5..a9429af7a0 100644
--- testcases/kernel/syscalls/move_mount/move_mount03.c
+++ testcases/kernel/syscalls/move_mount/move_mount03.c
@@ -5,8 +5,6 @@
  */
 
 /*\
- * [Description]
- *
  * Test allow to mount beneath top mount feature added in kernel 6.5:
  * 6ac392815628 ("fs: allow to mount beneath top mount")
  *
@@ -14,9 +12,10 @@
  * https://github.com/brauner/move-mount-beneath
  *
  * See also:
- * https://lore.kernel.org/all/20230202-fs-move-mount-replace-v4-0-98f3d80d7eaa@kernel.org/
- * https://lwn.net/Articles/930591/
- * https://github.com/brauner/move-mount-beneath
+ *
+ * - https://lore.kernel.org/all/20230202-fs-move-mount-replace-v4-0-98f3d80d7eaa@kernel.org/
+ * - https://lwn.net/Articles/930591/
+ * - https://github.com/brauner/move-mount-beneath
  */
 
 /*
@@ -68,7 +67,6 @@
  *           --/mnt    /DIRA /DIRB
  *         Check content of /DIRB, you can see file A exist since
  *         current /DIRB mount source is already become /DIRA
- *
  */
 
 #include <stdio.h>
@@ -142,4 +140,8 @@ static struct tst_test test = {
 	.needs_tmpdir = 1,
 	.setup = setup,
 	.cleanup = cleanup,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "6ac392815628f"},
+		{}
+	},
 };

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp