| Message ID | 20220428104642.110-1-chenhx.fnst@fujitsu.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v5] syscalls/mount_setattr01: Add basic functional test | expand |
Hi Dai, Chen, Reviewed-by: Petr Vorel <pvorel@suse.cz> LGTM, waiting for Cyril's final ack before merging. > diff --git a/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c b/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c ... > +#define MNTPOINT "mntpoint" > +#define OT_MNTPOINT "ot_mntpoint" > +#define TCASE_ENTRY(attrs, exp_attrs) \ > + { \ > + .name = #attrs, \ > + .mount_attrs = attrs, \ > + .expect_attrs = exp_attrs \ > + } nit: This is absolutely ok, but I'd personally use the version from v2, which set only .name and .mount_attrs (the only point for macros are to reduce duplicity, which is not in passing .expect_attrs). Kind regards, Petr
> -----邮件原件----- > 发件人: Petr Vorel <pvorel@suse.cz> > 发送时间: 2022年4月28日 20:24 > <daisl.fnst@fujitsu.com> > 主题: Re: [PATCH v5] syscalls/mount_setattr01: Add basic functional test > > Hi Dai, Chen, > > Reviewed-by: Petr Vorel <pvorel@suse.cz> LGTM, waiting for Cyril's final ack > before merging. Hi, Cyril Any comments? Regards, - Chen > > > diff --git a/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c > > b/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c > ... > > +#define MNTPOINT "mntpoint" > > +#define OT_MNTPOINT "ot_mntpoint" > > +#define TCASE_ENTRY(attrs, exp_attrs) \ > > + { \ > > + .name = #attrs, \ > > + .mount_attrs = attrs, \ > > + .expect_attrs = exp_attrs \ > > + } > nit: This is absolutely ok, but I'd personally use the version from v2, which set > only .name and .mount_attrs (the only point for macros are to reduce duplicity, > which is not in passing .expect_attrs). > > Kind regards, > Petr
Hi Chen, Dai, ... > +static void setup(void) > +{ > + fsopen_supported_by_kernel(); > + struct stat st = {0}; > + > + if (stat(OT_MNTPOINT, &st) == -1) > + SAFE_MKDIR(OT_MNTPOINT, 0777); > +} > + > +static void run(unsigned int n) > +{ > + 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)); > + if (!TST_PASS) > + return; > + > + otfd = (int)TST_RET; > + > + TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH, &attr, sizeof(attr)), > + "%s set", tc->name); > + if (!TST_PASS) > + goto out1; > + > + TST_EXP_PASS_SILENT(move_mount(otfd, "", AT_FDCWD, OT_MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH)); > + if (!TST_PASS) > + goto out1; > + mount_flag = 1; > + SAFE_CLOSE(otfd); > + > + TST_EXP_PASS_SILENT(statvfs(OT_MNTPOINT, &buf), "statvfs sucess"); > + if (!TST_PASS) > + goto out2; > + > + if (buf.f_flag & tc->expect_attrs) > + tst_res(TPASS, "%s is actually set as expected", tc->name); > + else > + tst_res(TFAIL, "%s is not actually set as expected", tc->name); > + > + goto out2; > + > +out1: > + SAFE_CLOSE(otfd); > +out2: > + mount_flag = 0; > + SAFE_UMOUNT(OT_MNTPOINT); mount_flag needs to be checked before otherwise if it fails on: TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH, &attr, sizeof(attr)), "%s set", tc->name); It tries to umount() not yet mounted filesystem: mount_setattr01.c:109: TPASS: MOUNT_ATTR_RDONLY is actually set as expected mount_setattr01.c:109: TPASS: MOUNT_ATTR_NOSUID is actually set as expected mount_setattr01.c:109: TPASS: MOUNT_ATTR_NODEV is actually set as expected mount_setattr01.c:109: TPASS: MOUNT_ATTR_NOEXEC is actually set as expected mount_setattr01.c:93: TFAIL: MOUNT_ATTR_NOSYMFOLLOW set failed: EINVAL (22) tst_device.c:394: TWARN: umount('ot_mntpoint') failed with EINVAL mount_setattr01.c:119: TBROK: umount(ot_mntpoint) failed: EINVAL (22) Because we mount later than opening otfd with open_tree() but need to close otfd before umount() we cannot depend only on labels. Thus I suggest to have this cleanup code: out1: SAFE_CLOSE(otfd); out2: if (mount_flag) SAFE_UMOUNT(OT_MNTPOINT); mount_flag = 0; If Cyril is ok with this I'd merge it before release. (I tested it on various systems, found only this issue.) Kind regards, Petr +++ testcases/kernel/syscalls/mount_setattr/mount_setattr01.c @@ -115,8 +115,10 @@ static void run(unsigned int n) out1: SAFE_CLOSE(otfd); out2: + if (mount_flag) + SAFE_UMOUNT(OT_MNTPOINT); + mount_flag = 0; - SAFE_UMOUNT(OT_MNTPOINT); } static struct tst_test test = {
Hi Petr > Hi Chen, Dai, > > ... >> +static void setup(void) >> +{ >> + fsopen_supported_by_kernel(); >> + struct stat st = {0}; >> + >> + if (stat(OT_MNTPOINT,&st) == -1) >> + SAFE_MKDIR(OT_MNTPOINT, 0777); >> +} >> + >> +static void run(unsigned int n) >> +{ >> + 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)); >> + if (!TST_PASS) >> + return; >> + >> + otfd = (int)TST_RET; >> + >> + TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH,&attr, sizeof(attr)), >> + "%s set", tc->name); >> + if (!TST_PASS) >> + goto out1; >> + >> + TST_EXP_PASS_SILENT(move_mount(otfd, "", AT_FDCWD, OT_MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH)); >> + if (!TST_PASS) >> + goto out1; >> + mount_flag = 1; >> + SAFE_CLOSE(otfd); >> + >> + TST_EXP_PASS_SILENT(statvfs(OT_MNTPOINT,&buf), "statvfs sucess"); >> + if (!TST_PASS) >> + goto out2; >> + >> + if (buf.f_flag& tc->expect_attrs) >> + tst_res(TPASS, "%s is actually set as expected", tc->name); >> + else >> + tst_res(TFAIL, "%s is not actually set as expected", tc->name); >> + >> + goto out2; >> + >> +out1: >> + SAFE_CLOSE(otfd); >> +out2: >> + mount_flag = 0; >> + SAFE_UMOUNT(OT_MNTPOINT); > mount_flag needs to be checked before otherwise if it fails on: > TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH,&attr, sizeof(attr)), > "%s set", tc->name); > > It tries to umount() not yet mounted filesystem: > > mount_setattr01.c:109: TPASS: MOUNT_ATTR_RDONLY is actually set as expected > mount_setattr01.c:109: TPASS: MOUNT_ATTR_NOSUID is actually set as expected > mount_setattr01.c:109: TPASS: MOUNT_ATTR_NODEV is actually set as expected > mount_setattr01.c:109: TPASS: MOUNT_ATTR_NOEXEC is actually set as expected > mount_setattr01.c:93: TFAIL: MOUNT_ATTR_NOSYMFOLLOW set failed: EINVAL (22) > tst_device.c:394: TWARN: umount('ot_mntpoint') failed with EINVAL > mount_setattr01.c:119: TBROK: umount(ot_mntpoint) failed: EINVAL (22) > > Because we mount later than opening otfd with open_tree() but need to close otfd > before umount() we cannot depend only on labels. > > Thus I suggest to have this cleanup code: > > out1: > SAFE_CLOSE(otfd); > out2: > if (mount_flag) > SAFE_UMOUNT(OT_MNTPOINT); > > mount_flag = 0; > > If Cyril is ok with this I'd merge it before release. > (I tested it on various systems, found only this issue.) This fix is obviously correct. Since I have reviewed this patch in internal, you can also add my reviewed-by for this patch. ps: I think we should also check functionality like mount03 did instead of just check flag. Best Regards Yang Xu > > Kind regards, > Petr > > +++ testcases/kernel/syscalls/mount_setattr/mount_setattr01.c > @@ -115,8 +115,10 @@ static void run(unsigned int n) > out1: > SAFE_CLOSE(otfd); > out2: > + if (mount_flag) > + SAFE_UMOUNT(OT_MNTPOINT); > + > mount_flag = 0; > - SAFE_UMOUNT(OT_MNTPOINT); > } > > static struct tst_test test = { >
Hi Xu, all, > > Thus I suggest to have this cleanup code: > > out1: > > SAFE_CLOSE(otfd); > > out2: > > if (mount_flag) > > SAFE_UMOUNT(OT_MNTPOINT); > > mount_flag = 0; > > If Cyril is ok with this I'd merge it before release. > > (I tested it on various systems, found only this issue.) > This fix is obviously correct. Since I have reviewed this patch in > internal, you can also add my reviewed-by for this patch. Adding it (I'm lazy thus want patchwork to take this patch. Reviewed-by: Yang Xu <xuyang2018.jy@fujitsu.com> > ps: I think we should also check functionality like mount03 did instead > of just check flag. +1. Although test_rwflag() is a bit complex. Kind regards, Petr > Best Regards > Yang Xu
Hi all, merged. I'm sorry Xu, I have forgotten to add your Reviewed-by. Kind regards, Petr
diff --git a/configure.ac b/configure.ac index 81cba98c2..816ebf820 100644 --- a/configure.ac +++ b/configure.ac @@ -220,6 +220,13 @@ AC_CHECK_TYPES([struct __kernel_old_timeval, struct __kernel_old_timespec, struc struct __kernel_old_itimerspec, struct __kernel_itimerspec],,,[#include <sys/socket.h>]) AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>]) +AC_CHECK_TYPES([struct mount_attr],,,[ +#ifdef HAVE_LINUX_MOUNT_H +# include <linux/mount.h> +#else +# include <sys/mount.h> +#endif +]) # Tools knobs diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h index 377af85ec..abf059486 100644 --- a/include/lapi/fsmount.h +++ b/include/lapi/fsmount.h @@ -16,6 +16,44 @@ #include "lapi/fcntl.h" #include "lapi/syscalls.h" +/* + * Mount attributes. + */ +#ifndef MOUNT_ATTR_RDONLY +# define MOUNT_ATTR_RDONLY 0x00000001 /* Mount read-only */ +#endif +#ifndef MOUNT_ATTR_NOSUID +# define MOUNT_ATTR_NOSUID 0x00000002 /* Ignore suid and sgid bits */ +#endif +#ifndef MOUNT_ATTR_NODEV +# define MOUNT_ATTR_NODEV 0x00000004 /* Disallow access to device special files */ +#endif +#ifndef MOUNT_ATTR_NOEXEC +# define MOUNT_ATTR_NOEXEC 0x00000008 /* Disallow program execution */ +#endif +#ifndef MOUNT_ATTR_NODIRATIME +# define MOUNT_ATTR_NODIRATIME 0x00000080 /* Do not update directory access times */ +#endif +#ifndef MOUNT_ATTR_NOSYMFOLLOW +# define MOUNT_ATTR_NOSYMFOLLOW 0x00200000 /* Do not follow symlinks */ +#endif + +#ifndef ST_NOSYMFOLLOW +# define ST_NOSYMFOLLOW 0x2000 /* do not follow symlinks */ +#endif + +#ifndef HAVE_STRUCT_MOUNT_ATTR +/* + * mount_setattr() + */ +struct mount_attr { + uint64_t attr_set; + uint64_t attr_clr; + uint64_t propagation; + uint64_t userns_fd; +}; +#endif + #ifndef HAVE_FSOPEN static inline int fsopen(const char *fsname, unsigned int flags) { @@ -62,6 +100,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 24d0f955d..3b26d19e6 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -803,6 +803,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 000000000..52a77b9ca --- /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 000000000..5ea7d67db --- /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 000000000..ef26bcbf2 --- /dev/null +++ b/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c @@ -0,0 +1,132 @@ +// 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: + * + * - MOUNT_ATTR_RDONLY - makes the mount read-only + * - 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. + * - MOUNT_ATTR_NODEV - prevents access to devices on this mount + * - MOUNT_ATTR_NOEXEC - prevents executing programs on this mount + * - MOUNT_ATTR_NOSYMFOLLOW - prevents following symbolic links + * on this mount + * - MOUNT_ATTR_NODIRATIME - prevents updating access time for + * directories on this mount + * + * The functionality was added in v5.12. + */ + +#define _GNU_SOURCE + +#include <sys/statvfs.h> +#include "tst_test.h" +#include "lapi/fsmount.h" +#include "lapi/stat.h" + +#define MNTPOINT "mntpoint" +#define OT_MNTPOINT "ot_mntpoint" +#define TCASE_ENTRY(attrs, exp_attrs) \ + { \ + .name = #attrs, \ + .mount_attrs = attrs, \ + .expect_attrs = exp_attrs \ + } + +static int mount_flag, otfd = -1; + +static struct tcase { + char *name; + unsigned int mount_attrs; + unsigned int expect_attrs; +} tcases[] = { + TCASE_ENTRY(MOUNT_ATTR_RDONLY, ST_RDONLY), + TCASE_ENTRY(MOUNT_ATTR_NOSUID, ST_NOSUID), + TCASE_ENTRY(MOUNT_ATTR_NODEV, ST_NODEV), + TCASE_ENTRY(MOUNT_ATTR_NOEXEC, ST_NOEXEC), + TCASE_ENTRY(MOUNT_ATTR_NOSYMFOLLOW, ST_NOSYMFOLLOW), + TCASE_ENTRY(MOUNT_ATTR_NODIRATIME, ST_NODIRATIME), +}; + +static void cleanup(void) +{ + if (otfd > -1) + SAFE_CLOSE(otfd); + if (mount_flag) + SAFE_UMOUNT(OT_MNTPOINT); +} + +static void setup(void) +{ + fsopen_supported_by_kernel(); + struct stat st = {0}; + + if (stat(OT_MNTPOINT, &st) == -1) + SAFE_MKDIR(OT_MNTPOINT, 0777); +} + +static void run(unsigned int n) +{ + 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)); + if (!TST_PASS) + return; + + otfd = (int)TST_RET; + + TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH, &attr, sizeof(attr)), + "%s set", tc->name); + if (!TST_PASS) + goto out1; + + TST_EXP_PASS_SILENT(move_mount(otfd, "", AT_FDCWD, OT_MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH)); + if (!TST_PASS) + goto out1; + mount_flag = 1; + SAFE_CLOSE(otfd); + + TST_EXP_PASS_SILENT(statvfs(OT_MNTPOINT, &buf), "statvfs sucess"); + if (!TST_PASS) + goto out2; + + if (buf.f_flag & tc->expect_attrs) + tst_res(TPASS, "%s is actually set as expected", tc->name); + else + tst_res(TFAIL, "%s is not actually set as expected", tc->name); + + goto out2; + +out1: + SAFE_CLOSE(otfd); +out2: + mount_flag = 0; + SAFE_UMOUNT(OT_MNTPOINT); +} + +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}, +};