Re: [LTP] [PATCH v2] syscalls/mount_setattr01: Add basic functional test

[Petr Vorel <pvorel@suse.cz>]

Hi Chen, Dai,

> From: Chen Hanxiao <chenhx.fnst@fujitsu.com>

> diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
...
> +/*
> + * mount_setattr()
> + */
> +struct mount_attr {
> +	uint64_t attr_set;
> +	uint64_t attr_clr;
> +	uint64_t propagation;
> +	uint64_t userns_fd;
> +};
Interesting enough: in kernel tools/testing/selftests/mount_setattr/mount_setattr_test.c
defines it as __u64 (IMHO should be really uint64_t as that test is userspace as
Cyril pointed out) but real kernel code in fs/namespace.c happily uses "unsigned
int" :).

...
> diff --git a/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c b/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c
> new file mode 100644
> index 000000000..d63db46fa
> --- /dev/null
> +++ b/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
> + * Author: Dai Shili <daisl.fnst@fujitsu.com>
> + * Author: Chen Hanxiao <chenhx.fnst@fujitsu.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Basic mount_setattr() test.
> + * Test whether the basic mount attributes are set correctly.
> + *
> + * Verify some MOUNT_SETATTR(2) attributes:
> + * 1) MOUNT_ATTR_RDONLY - makes the mount read-only
> + * 2) MOUNT_ATTR_NOSUID - causes the mount not to honor the
> + *    set-user-ID and set-group-ID mode bits and file capabilities
> + *    when executing programs.
> + * 3) MOUNT_ATTR_NODEV - prevents access to devices on this mount
> + * 4) MOUNT_ATTR_NOEXEC - prevents executing programs on this mount
> + * 5) MOUNT_ATTR_NOSYMFOLLOW - prevents following symbolic links
> + *    on this mount
> + * 6) MOUNT_ATTR_NODIRATIME - prevents updating access time for
> + *    directories on this mount
> + * Minimum Linux version required is v5.12.
Since we don't check for v5.12, it might be better to state "The functionality
was added in v5.12." (although only enterprise kernels would backport new
functionality, mainline stable kernels will not).
> + */

This needs some changes in order to be formatted properly as list and have
paragraphs. Sigh, nobody really runs:

cd metadata/ && make && chromium ../docparse/metadata.html

to have look what output his docs has :(
(can be fixed before merge)

/*\
 * [Description]
 *
 * Basic mount_setattr() test.
 * Test whether the basic mount attributes are set correctly.
 *
 * Verify some MOUNT_SETATTR(2) attributes:
 *
 * 1. MOUNT_ATTR_RDONLY &ndash; makes the mount read-only
 * 2. MOUNT_ATTR_NOSUID &ndash; causes the mount not to honor the
 *    set-user-ID and set-group-ID mode bits and file capabilities
 *    when executing programs.
 * 3. MOUNT_ATTR_NODEV &ndash; prevents access to devices on this mount
 * 4. MOUNT_ATTR_NOEXEC &ndash; prevents executing programs on this mount
 * 5. MOUNT_ATTR_NOSYMFOLLOW &ndash; prevents following symbolic links
 *    on this mount
 * 6. MOUNT_ATTR_NODIRATIME &ndash; prevents updating access time for
 *    directories on this mount
 *
 * Minimum Linux version required is v5.12.
 */

...
> +static void run(unsigned int n)
> +{
> +	int otfd;
> +	struct tcase *tc = &tcases[n];
> +	struct mount_attr attr = {
> +		.attr_set = tc->mount_attrs,
> +	};
> +	struct statvfs buf;
> +
> +	TST_EXP_FD_SILENT(open_tree(AT_FDCWD, MNTPOINT, AT_EMPTY_PATH |
> +		AT_SYMLINK_NOFOLLOW | OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE));
Although Cyril mentioned only TST_EXP_FD_SILENT(), IMHO it should be followed with:
	if (!TST_PASS)
		return;

Or does it make sense to continue testing when open_tree() fails?

> +	otfd = (int)TST_RET;
> +
> +	TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH, &attr, sizeof(attr)),
> +		"%s set", tc->name);

Shouldn't be here also :
	if (!TST_PASS)
		return;

check? I guess we need SAFE_ variants for not having to check it all the time.

Or does it make sense to continue testing when mount_setattr() fails?

> +	TEST(move_mount(otfd, "", AT_FDCWD, OT_MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH));
> +
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "move_mount() failed");
> +		return;
> +	}

Maybe instead of TEST() and if use this?

	TST_EXP_PASS_SILENT(move_mount(otfd, "", AT_FDCWD, OT_MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH));

	if (!TST_PASS)
		return;

> +
> +	SAFE_CLOSE(otfd);
> +
> +	TST_EXP_PASS_SILENT(statvfs(OT_MNTPOINT, &buf), "statvfs sucess");
And here as well.
> +
> +	if ((buf.f_flag & tc->expect_attrs) == 0)
> +		tst_res(TFAIL, "%s is not actually set as expected", tc->name);
> +	else
> +		tst_res(TPASS, "%s is actually set as expected", tc->name);
> +
> +	if (tst_is_mounted_at_tmpdir(OT_MNTPOINT))
> +		SAFE_UMOUNT(OT_MNTPOINT);
> +}

The rest LGTM.

Kind regards,
Petr

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