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

[Petr Vorel <pvorel@suse.cz>]

> 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