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

[Petr Vorel <pvorel@suse.cz>]

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