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

[chenhx.fnst@fujitsu.com]

Hi

> -----邮件原件-----
> 发件人: ltp <ltp-bounces+chenhx.fnst=cn.fujitsu.com@lists.linux.it> 代表 Cyril
> Hrubis
> 发送时间: 2022年3月10日 21:06
> 收件人: Dai, Shili <daisl.fnst@fujitsu.com>
> 抄送: ltp@lists.linux.it
> 主题: Re: [LTP] [PATCH] syscalls/mount_setattr01: Add basic functional test
> 
> 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
> 

Get it.
> ...
> 
> > +/*
> > + * 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.
> 
OK.

> > +};
...
> > +#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.
> 
Sure.

> > +
> > +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.
> 

There are several file systems to be tested. If we do not rmdir, it will be error(dir is EEXIST) when testing the second file system.

> > +}
> > +
> > +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()
OK.

> 
> > +	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()
> 
OK.

> 
> 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.
> 

Guess statvfs(3) may help.

> > +	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.
> 
Will be deleted in v2.

> > +	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().

move_mount place the filesystem into a mount namespace and then it will be visible to users.

I'll post a v2 patch soon.

Regards,
- Chen

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