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

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

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,
> +};

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,
> +};