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

[Richard Palethorpe <rpalethorpe@suse.de>]

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