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

* Re: [LTP] [PATCH] syscalls/mount_setattr01: Add basic functional test
  2022-03-10 20:19 [LTP] [PATCH] syscalls/mount_setattr01: Add basic functional test Dai Shili
@ 2022-03-10 13:06 ` Cyril Hrubis
  2022-04-18 10:14   ` [LTP] 回复: " chenhx.fnst
  2022-04-19  2:08   ` chenhx.fnst
  0 siblings, 2 replies; 4+ messages in thread
From: Cyril Hrubis @ 2022-03-10 13:06 UTC (permalink / raw)
  To: Dai Shili; +Cc: ltp

Hi!
> diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
> index fa25306..99d0a0a 100644
> --- a/include/lapi/fsmount.h
> +++ b/include/lapi/fsmount.h
> @@ -15,6 +15,26 @@
>  #include "lapi/fcntl.h"
>  #include "lapi/syscalls.h"
>  
> +/*
> + * Mount attributes.
> + */
> +#define MOUNT_ATTR_RDONLY       0x00000001 /* Mount read-only */
> +#define MOUNT_ATTR_NOSUID       0x00000002 /* Ignore suid and sgid bits */
> +#define MOUNT_ATTR_NODEV        0x00000004 /* Disallow access to device special files */
> +#define MOUNT_ATTR_NOEXEC       0x00000008 /* Disallow program execution */
> +#define MOUNT_ATTR_NODIRATIME   0x00000080 /* Do not update directory access times */
> +#define MOUNT_ATTR_NOSYMFOLLOW  0x00200000 /* Do not follow symlinks */

These have to be defined only if they are missing, otherwise we will end
up with redefition warnings once glibc add these constants, i.e.

#ifndef MOUNT_ATTR_RDONLY
# define MOUNT_ATTR_RDONLY 0x00000001
#endif

...

> +/*
> + * mount_setattr()
> + */
> +struct mount_attr {
> +	__u64 attr_set;
> +	__u64 attr_clr;
> +	__u64 propagation;
> +	__u64 userns_fd;

These should be uint64_t, we cannot use the kernel types in userspace.

> +};
> +
>  #ifndef HAVE_FSOPEN
>  static inline int fsopen(const char *fsname, unsigned int flags)
>  {
> @@ -61,6 +81,15 @@ static inline int open_tree(int dirfd, const char *pathname, unsigned int flags)
>  }
>  #endif /* HAVE_OPEN_TREE */
>  
> +#ifndef HAVE_MOUNT_SETATTR
> +static inline int mount_setattr(int dirfd, const char *from_pathname, unsigned int flags,
> +				struct mount_attr *attr, size_t size)
> +{
> +	return tst_syscall(__NR_mount_setattr, dirfd, from_pathname, flags,
> +			   attr, size);
> +}
> +#endif /* HAVE_MOUNT_SETATTR */
> +
>  /*
>   * New headers added in kernel after 5.2 release, create them for old userspace.
>  */
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 6186bfc..1a47a2e 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -800,6 +800,8 @@ mount04 mount04
>  mount05 mount05
>  mount06 mount06
>  
> +mount_setattr01 mount_setattr01
> +
>  move_mount01 move_mount01
>  move_mount02 move_mount02
>  
> diff --git a/testcases/kernel/syscalls/mount_setattr/.gitignore b/testcases/kernel/syscalls/mount_setattr/.gitignore
> new file mode 100644
> index 0000000..52a77b9
> --- /dev/null
> +++ b/testcases/kernel/syscalls/mount_setattr/.gitignore
> @@ -0,0 +1 @@
> +/mount_setattr01
> diff --git a/testcases/kernel/syscalls/mount_setattr/Makefile b/testcases/kernel/syscalls/mount_setattr/Makefile
> new file mode 100644
> index 0000000..5ea7d67
> --- /dev/null
> +++ b/testcases/kernel/syscalls/mount_setattr/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> 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 0000000..b4b1d85
> --- /dev/null
> +++ b/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
> + * Author: Dai Shili <daisl.fnst@fujitsu.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Basic mount_setattr() test.
> + * Test whether the basic mount attributes are set and cleared correctly.
> + *
> + * Minimum Linux version required is v5.12.
> + */
> +
> +#include "tst_test.h"
> +#include "lapi/fsmount.h"
> +#include "lapi/stat.h"
> +
> +#define MNTPOINT        "mntpoint"
> +#define OT_MNTPOINT     "ot_mntpoint"
> +#define TCASE_ENTRY(_mount_attrs)	{.name = #_mount_attrs, .mount_attrs = _mount_attrs}
                          ^
Please do not use identifiers starting with underscore, these are
reserved for kernel and libc.

> +
> +static int dir_created;
> +
> +static struct tcase {
> +	char *name;
> +	unsigned int mount_attrs;
> +} tcases[] = {
> +	TCASE_ENTRY(MOUNT_ATTR_RDONLY),
> +	TCASE_ENTRY(MOUNT_ATTR_NOSUID),
> +	TCASE_ENTRY(MOUNT_ATTR_NODEV),
> +	TCASE_ENTRY(MOUNT_ATTR_NOEXEC),
> +	TCASE_ENTRY(MOUNT_ATTR_NOSYMFOLLOW),
> +	TCASE_ENTRY(MOUNT_ATTR_NODIRATIME),
> +};
> +
> +static void cleanup(void)
> +{
> +	if (dir_created)
> +		SAFE_RMDIR(OT_MNTPOINT);

No need to rmdir anything, it will be removed by the test library along
with the test temporary directory.

> +}
> +
> +static void setup(void)
> +{
> +	fsopen_supported_by_kernel();
> +	SAFE_MKDIR(OT_MNTPOINT, 0777);
> +	dir_created = 1;
> +}
> +
> +static void run(unsigned int n)
> +{
> +	int otfd;
> +	struct tcase *tc = &tcases[n];
> +	struct mount_attr attr = {
> +		.attr_set = tc->mount_attrs,
> +	};
> +
> +	TEST(otfd = open_tree(AT_FDCWD, MNTPOINT, AT_EMPTY_PATH |
> +		AT_SYMLINK_NOFOLLOW | OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE));
> +	if (otfd == -1) {
> +		tst_res(TFAIL | TTERRNO, "open_tree() failed");
> +		return;
> +	}

This can be just TST_EXP_FD()

> +	TEST(mount_setattr(otfd, "", AT_EMPTY_PATH, &attr, sizeof(attr)));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "mount_setattr() set attr %s failed.", tc->name);
> +		return;
> +	}

and this can be TST_EXP_PASS()

And here we should check if the attribute was really changed.

I guess that we can add functions to verify most of the attributes, e.g.
for RDONLY we can add a function that tries to open file for writing and
expects that it fails with EROFS if the attribute has been set.

> +	attr.attr_clr = tc->mount_attrs;

This does not have any efect at all unless we also reset attr_set.
Accordinlgy to the manual page kernel first clears attributes then sets
them, so as long as the attr_set remains set to tc->mount_attrs the
following mount_setattr() is no-op.

> +	TEST(mount_setattr(otfd, "", AT_EMPTY_PATH, &attr, sizeof(attr)));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "mount_setattr() clear attr %s failed.", tc->name);
> +		return;
> +	}

This can be just TST_EXP_PASS()

And here we should check that the attribute has been reset, e.g. for
RDONLY attribute we should check that files can be opened for writing
again.

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

I do not get why we do move the mount at all, this is supposed to be a
simple test for mount_setattr().

> +	SAFE_CLOSE(otfd);
> +
> +	if (tst_is_mounted_at_tmpdir(OT_MNTPOINT)) {
> +		SAFE_UMOUNT(OT_MNTPOINT);
> +		tst_res(TPASS, "mount_setattr() set and clear attr %s passed.", tc->name);
> +	}
> +}
> +
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.mount_device = 1,
> +	.mntpoint = MNTPOINT,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *const []){"fuse", NULL},
> +};
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 4+ messages in thread