Message ID | 20220715062519.2480-1-chenhx.fnst@fujitsu.com |
---|---|
State | Changes Requested |
Headers | show |
Series | mount03: Convert to new API | expand |
Hi Chen, quite nice work, but more cleanup is needed. > testcases/kernel/syscalls/mount/mount03.c | 462 ++++++++-------------- ... > #define FILE_MODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) > -#define DIR_MODE (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \ > - S_IXGRP|S_IROTH|S_IXOTH) > #define SUID_MODE (S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH) @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not like permission warnings. Do we want to follow? Or should we remove these from our checkpatch.pl fork (we use constants in many places)? $ make check-mount03 mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'. mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'. mount03.c:50: WARNING: static char array declaration should probably be static const char mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. ... > +#define MNTPOINT "mntpoint" ... > +static char nobody_uid[] = "nobody"; I'd remove constant and use "nobody" in the setup (the same like creat08.c). If we want to have it defined, I'd use #define (like you use it in MNTPOINT). Also your code produces warning: mount03.c:50: WARNING: static char array declaration should probably be static const char ... > +static int test_ms_nosuid(void) ... > + int pid, status; pid_t pid; // don't use int for pid please. This is not needed (-1 is handled by SAFE_FORK()), therefore you don't need switch. For a readability, your code (most of inherited from the original) is: static int test_ms_nosuid(void) { int pid, status; switch (pid = SAFE_FORK()) { case -1: tst_brk(TFAIL | TERRNO, "fork failed"); break; case 0: SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid); execlp(file, basename(file), NULL); exit(1); default: waitpid(pid, &status, 0); if (WIFEXITED(status)) { /* reset the setup_uid */ if (status) return 0; } return 1; } return 0; } * I'm not sure why exit(1), * we have SAFE_EXECLP. Therefore something like: static void test_ms_nosuid(void) { int status; pid_t pid; pid = SAFE_FORK(); if (!pid) { SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid); SAFE_EXECLP(file, basename(file), NULL); } SAFE_WAITPID(pid, &status, 0); } But SAFE_WAITPID() would not allow to print TPASS/TFAIL. > -/* > - * test_rwflag(int i, int cnt) > - * Validate the mount system call for rwflags. > - */ > -int test_rwflag(int i, int cnt) > +static void test_rwflag(int i) Instead of a crazy long switch which is in the original code, we prefer to use functions which are put into struct tcase via function pointer. See e.g. mq_open01.c for example. ... > + snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT); > + TEST(test_ms_nosuid()); Please don't reuse TEST() macro for normal function. Just call it: > + if (TST_RET == 0) > + tst_res(TPASS, "mount(2) passed with flag MS_NOSUID"); > + else > + tst_res(TFAIL, "mount(2) failed with flag MS_NOSUID"); There is TST_EXP_PASS() macro: TST_EXP_PASS(test_ms_nosuid()) if (!TST_PASS) return; But, please don't reuse TEST() macro for normal function. These are meant to be for syscalls testing, here I'd print TPASS/TFAIL in test_ms_nosuid() and make it void. ... > +static void setup(void) > +{ > + struct stat file_stat = {0}; > + ltpuser = getpwnam(nobody_uid); ltpuser = SAFE_GETPWNAM("nobody"); (or use constant, but SAFE_GETPWNAM is required (no check, see creat08.c) > + if (ltpuser == NULL) > + tst_res(TFAIL, "getpwnam failed"); > + snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT); > + SAFE_STAT(MNTPOINT, &file_stat); > if (file_stat.st_mode != SUID_MODE && > - chmod(file, SUID_MODE) < 0) > - tst_brkm(TBROK, cleanup, > - "setuid for setuid_test failed"); > - SAFE_UMOUNT(cleanup, mntpoint); > - > - TEST_PAUSE; > + chmod(MNTPOINT, SUID_MODE) < 0) > + tst_res(TFAIL, "setuid for setuid_test failed"); Please use tst_brk(TBROK, ...), original version has tst_brkm(TBROK, ...) for a reason [1]: 1) tst_brk quits testing, which is usually required in setup function 2) TBROK is usually used instead of TFAIL in setup function TBROK: Something has failed in test preparation phase. TFAIL: Test has failed. (This description should have been in LTP Test Writing Guidelines [2], because it applies to both C and shell API.) ... > +static struct tst_test test = { > + .tcnt = ARRAY_SIZE(tcases), > + .test = run, > + .setup = setup, > + .cleanup = cleanup, > + .needs_root = 1, > + .format_device = 1, > + .resource_files = resource_files, Please inline resource files like: .resource_files = (const char *const[]) { "mount03_setuid_test", NULL, }, It produces "mount03_setuid_test" in doc [3]: | .resource_files | mount03_setuid_test | But using .resource_files = resource_files produces just "resource_files": | .resource_files | resource_files | (to make doc locally: cd metadata && make # result is in ../docparse/metadata.html) > + .forks_child = 1, > + .mntpoint = MNTPOINT, > + .skip_filesystems = (const char *const []){"fuse", NULL}, Why you introduced skipping on FUSE? I don't see it in the old code, nothing mentioned in commit message). Also when I test it for all filesystems (which could be a good idea for mount test) it fails for vfat, exfat and ntfs: .skip_filesystems = (const char *const []){"vfat", "exfat", "ntfs", NULL}, .all_filesystems = 1, Skipping just fuse does not help in my case: tst_test.c:1599: TINFO: Testing on ntfs tst_test.c:1064: TINFO: Formatting /dev/loop3 with ntfs opts='' extra opts='' The partition start sector was not specified for /dev/loop3 and it could not be obtained automatically. It has been set to 0. The number of sectors per track was not specified for /dev/loop3 and it could not be obtained automatically. It has been set to 0. The number of heads was not specified for /dev/loop3 and it could not be obtained automatically. It has been set to 0. To boot from a device, Windows needs the 'partition start sector', the 'sectors per track' and the 'number of heads' to be set. Windows will not be able to boot from this device. mount03.c:102: TFAIL: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected succeeded tst_device.c:394: TWARN: umount('mntpoint') failed with EINVAL mount03.c:237: TBROK: umount(mntpoint) failed: EINVAL (22) tst_test.c:1599: TINFO: Testing on exfat tst_test.c:1064: TINFO: Formatting /dev/loop2 with exfat opts='' extra opts='' mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected : EROFS (30) mount03.c:112: TBROK: mknod() failed: EPERM (1) tst_test.c:1599: TINFO: Testing on vfat tst_test.c:1064: TINFO: Formatting /dev/loop1 with vfat opts='' extra opts='' mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected : EROFS (30) mount03.c:112: TBROK: mknod() failed: EPERM (1) Kind regards, Petr [1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#12-basic-test-interface [2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines [3] http://linux-test-project.github.io/metadata/metadata.stable.html
Hello Petr, Petr Vorel <pvorel@suse.cz> writes: > Hi Chen, > > quite nice work, but more cleanup is needed. > >> testcases/kernel/syscalls/mount/mount03.c | 462 ++++++++-------------- > ... >> #define FILE_MODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) >> -#define DIR_MODE (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \ >> - S_IXGRP|S_IROTH|S_IXOTH) >> #define SUID_MODE (S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH) > > @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not > like permission warnings. Do we want to follow? Or should we remove these from > our checkpatch.pl fork (we use constants in many places)? I'm not sure about the rule. However there is no reason we can't follow it for new tests. We don't want to have arbitrary style differences between LTP and the kernel. Some rules don't make sense in userland for technical reasons, but this one is purely about readability.
Hi! > @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not > like permission warnings. Do we want to follow? Or should we remove these from > our checkpatch.pl fork (we use constants in many places)? > > $ make check-mount03 > mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'. > mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'. > mount03.c:50: WARNING: static char array declaration should probably be static const char > mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. To be honest I think Linus is right at this one, the single octal number is way more readable than the bitwise or of four constants, so I would be inclined to start following the kernel practice here.
> Hi! > > @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not > > like permission warnings. Do we want to follow? Or should we remove these from > > our checkpatch.pl fork (we use constants in many places)? > > $ make check-mount03 > > mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'. > > mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'. > > mount03.c:50: WARNING: static char array declaration should probably be static const char > > mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > > mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > > mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > > mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > > mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > To be honest I think Linus is right at this one, the single octal number > is way more readable than the bitwise or of four constants, so I would > be inclined to start following the kernel practice here. Agree with the rule, numbers are indeed much readable and I'm for using it in LTP source. My concern was different: aren't these constants part of POSIX? See man from <sys/stat.h> from 1997 [1]. There might be a test for these constants but it has much lower priority than tests for new kernel functionality and CVE. Kind regards, Petr [1] https://pubs.opengroup.org/onlinepubs/007908799/xsh/sysstat.h.html
> -----邮件原件----- > > Hi Chen, > > quite nice work, but more cleanup is needed. > > > testcases/kernel/syscalls/mount/mount03.c | 462 > > ++++++++-------------- > ... > > #define FILE_MODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) > > -#define DIR_MODE (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \ > > - S_IXGRP|S_IROTH|S_IXOTH) > > #define SUID_MODE (S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH) > > @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does > not like permission warnings. Do we want to follow? Or should we remove these > from our checkpatch.pl fork (we use constants in many places)? > OK, will be fixed. > $ make check-mount03 > mount03.c:29: WARNING: Symbolic permissions > 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal > permissions '0644'. > mount03.c:30: WARNING: Symbolic permissions > 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal > permissions '0511'. > mount03.c:50: WARNING: static char array declaration should probably be > static const char > mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. > Consider using octal permissions '0700'. > mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. > Consider using octal permissions '0700'. > mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. > Consider using octal permissions '0700'. > mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. > Consider using octal permissions '0700'. > mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. > Consider using octal permissions '0700'. > > ... > > +#define MNTPOINT "mntpoint" > ... > > +static char nobody_uid[] = "nobody"; > I'd remove constant and use "nobody" in the setup (the same like creat08.c). > If we want to have it defined, I'd use #define (like you use it in MNTPOINT). > > Also your code produces warning: > mount03.c:50: WARNING: static char array declaration should probably be > static const char > > ... > > +static int test_ms_nosuid(void) > ... > > + int pid, status; > pid_t pid; // don't use int for pid please. > > This is not needed (-1 is handled by SAFE_FORK()), therefore you don't need > switch. > > For a readability, your code (most of inherited from the original) is: > static int test_ms_nosuid(void) > { > int pid, status; > > switch (pid = SAFE_FORK()) { > case -1: > tst_brk(TFAIL | TERRNO, "fork failed"); > break; > case 0: > SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid); > > execlp(file, basename(file), NULL); > exit(1); > default: > waitpid(pid, &status, 0); > if (WIFEXITED(status)) { > /* reset the setup_uid */ > if (status) > return 0; > } > return 1; > } > return 0; > } > > * I'm not sure why exit(1), > * we have SAFE_EXECLP. Therefore something like: > > > static void test_ms_nosuid(void) > { > int status; > pid_t pid; > > pid = SAFE_FORK(); > > if (!pid) { > SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid); > SAFE_EXECLP(file, basename(file), NULL); > } > > SAFE_WAITPID(pid, &status, 0); > } > > But SAFE_WAITPID() would not allow to print TPASS/TFAIL. Maybe we can use the original waitpid + tst_res logic. > > > -/* > > - * test_rwflag(int i, int cnt) > > - * Validate the mount system call for rwflags. > > - */ > > -int test_rwflag(int i, int cnt) > > +static void test_rwflag(int i) > Instead of a crazy long switch which is in the original code, we prefer to use > functions which are put into struct tcase via function pointer. See e.g. > mq_open01.c for example. > Thanks for the advice, will be fixed. > > ... > > + snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT); > > + TEST(test_ms_nosuid()); > Please don't reuse TEST() macro for normal function. Just call it: > > > + if (TST_RET == 0) > > + tst_res(TPASS, "mount(2) passed with flag MS_NOSUID"); > > + else > > + tst_res(TFAIL, "mount(2) failed with flag MS_NOSUID"); > There is TST_EXP_PASS() macro: > TST_EXP_PASS(test_ms_nosuid()) > if (!TST_PASS) > return; > > But, please don't reuse TEST() macro for normal function. These are meant to > be for syscalls testing, here I'd print TPASS/TFAIL in test_ms_nosuid() and make > it void. > OK. > ... > > +static void setup(void) > > +{ > > + struct stat file_stat = {0}; > > > + ltpuser = getpwnam(nobody_uid); > ltpuser = SAFE_GETPWNAM("nobody"); > (or use constant, but SAFE_GETPWNAM is required (no check, see creat08.c) > > > + if (ltpuser == NULL) > > + tst_res(TFAIL, "getpwnam failed"); > > + snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT); > > > + SAFE_STAT(MNTPOINT, &file_stat); > > if (file_stat.st_mode != SUID_MODE && > > - chmod(file, SUID_MODE) < 0) > > - tst_brkm(TBROK, cleanup, > > - "setuid for setuid_test failed"); > > - SAFE_UMOUNT(cleanup, mntpoint); > > - > > - TEST_PAUSE; > > + chmod(MNTPOINT, SUID_MODE) < 0) > > + tst_res(TFAIL, "setuid for setuid_test failed"); > Please use tst_brk(TBROK, ...), original version has tst_brkm(TBROK, ...) for a > reason [1]: > 1) tst_brk quits testing, which is usually required in setup function > 2) TBROK is usually used instead of TFAIL in setup function > > TBROK: Something has failed in test preparation phase. > TFAIL: Test has failed. > > (This description should have been in LTP Test Writing Guidelines [2], because it > applies to both C and shell API.) > Sure, I'll read the docs more carefully. > > ... > > +static struct tst_test test = { > > + .tcnt = ARRAY_SIZE(tcases), > > + .test = run, > > + .setup = setup, > > + .cleanup = cleanup, > > + .needs_root = 1, > > + .format_device = 1, > > + .resource_files = resource_files, > > Please inline resource files like: > .resource_files = (const char *const[]) { > "mount03_setuid_test", > NULL, > }, > > It produces "mount03_setuid_test" in doc [3]: > > | .resource_files | mount03_setuid_test | > > But using .resource_files = resource_files produces just "resource_files": > | .resource_files | resource_files | > > (to make doc locally: cd metadata && make # result is > in ../docparse/metadata.html) > > > + .forks_child = 1, > > + .mntpoint = MNTPOINT, > > + .skip_filesystems = (const char *const []){"fuse", NULL}, > Why you introduced skipping on FUSE? I don't see it in the old code, nothing > mentioned in commit message). > > Also when I test it for all filesystems (which could be a good idea for mount > test) it fails for vfat, exfat and ntfs: My test machine(Centos Stream) ntfs is provided by fuse, so it was skipped... I'll add them to skip list. Regards, - Chen > > .skip_filesystems = (const char *const []){"vfat", "exfat", "ntfs", NULL}, > .all_filesystems = 1, > > Skipping just fuse does not help in my case: > > tst_test.c:1599: TINFO: Testing on ntfs > tst_test.c:1064: TINFO: Formatting /dev/loop3 with ntfs opts='' extra opts='' > The partition start sector was not specified for /dev/loop3 and it could not be > obtained automatically. It has been set to 0. > The number of sectors per track was not specified for /dev/loop3 and it could > not be obtained automatically. It has been set to 0. > The number of heads was not specified for /dev/loop3 and it could not be > obtained automatically. It has been set to 0. > To boot from a device, Windows needs the 'partition start sector', the 'sectors > per track' and the 'number of heads' to be set. > Windows will not be able to boot from this device. > mount03.c:102: TFAIL: mount(2) passed with flag MS_RDONLY: open fail with > EROFS as expected succeeded > tst_device.c:394: TWARN: umount('mntpoint') failed with EINVAL > mount03.c:237: TBROK: umount(mntpoint) failed: EINVAL (22) > > tst_test.c:1599: TINFO: Testing on exfat > tst_test.c:1064: TINFO: Formatting /dev/loop2 with exfat opts='' extra opts='' > mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with > EROFS as expected : EROFS (30) > mount03.c:112: TBROK: mknod() failed: EPERM (1) > > tst_test.c:1599: TINFO: Testing on vfat > tst_test.c:1064: TINFO: Formatting /dev/loop1 with vfat opts='' extra opts='' > mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with > EROFS as expected : EROFS (30) > mount03.c:112: TBROK: mknod() failed: EPERM (1) > > Kind regards, > Petr > > [1] > https://github.com/linux-test-project/ltp/wiki/C-Test-API#12-basic-test-interfac > e > [2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines > [3] http://linux-test-project.github.io/metadata/metadata.stable.html
Hi All >> Hi! >>> @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not >>> like permission warnings. Do we want to follow? Or should we remove these from >>> our checkpatch.pl fork (we use constants in many places)? > >>> $ make check-mount03 >>> mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'. >>> mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'. >>> mount03.c:50: WARNING: static char array declaration should probably be static const char >>> mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. >>> mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. >>> mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. >>> mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. >>> mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'. > >> To be honest I think Linus is right at this one, the single octal number >> is way more readable than the bitwise or of four constants, so I would >> be inclined to start following the kernel practice here. > > Agree with the rule, numbers are indeed much readable and I'm for using it in > LTP source. Linux checkpatch.pl only think perms should use octal number, but not inlcude sticky bits, set-uid bit setgid bit and file types[1]. Should we modify checkpatch.pl to print only three bit instead of four bits because setgid bit setuid bit sticky bit are stil keep symbolic names . [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc1&id=f90774e1fd2700de4a6e0d62866d34a26c544bd0 Best Regards Yang Xu > > My concern was different: aren't these constants part of POSIX? See man from > <sys/stat.h> from 1997 [1]. There might be a test for these constants but > it has much lower priority than tests for new kernel functionality and CVE. > > Kind regards, > Petr > > [1] https://pubs.opengroup.org/onlinepubs/007908799/xsh/sysstat.h.html >
diff --git a/testcases/kernel/syscalls/mount/mount03.c b/testcases/kernel/syscalls/mount/mount03.c index 25f99bbfc..905fcbbac 100644 --- a/testcases/kernel/syscalls/mount/mount03.c +++ b/testcases/kernel/syscalls/mount/mount03.c @@ -1,136 +1,97 @@ +// SPDX-License-Identifier: GPL-2.0-or-later /* + * Copyright (c) Linux Test Project, 2022 * Copyright (c) Wipro Technologies Ltd, 2002. All Rights Reserved. - * - * This program is free software; you can redistribute it and/or modify it - * under the terms of version 2 of the GNU General Public License as - * published by the Free Software Foundation. - * - * This program is distributed in the hope that it would be useful, but - * WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. - * - * You should have received a copy of the GNU General Public License along - * with this program; if not, write the Free Software Foundation, Inc., - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. - * */ -/* - * DESCRIPTION - * Check for basic mount(2) system call flags. +/*\ + * [Description] + * + * Check for basic mount(2) system call flags. * - * Verify that mount(2) syscall passes for each flag setting and validate - * the flags - * 1) MS_RDONLY - mount read-only. - * 2) MS_NODEV - disallow access to device special files. - * 3) MS_NOEXEC - disallow program execution. - * 4) MS_SYNCHRONOUS - writes are synced at once. - * 5) MS_REMOUNT - alter flags of a mounted FS. - * 6) MS_NOSUID - ignore suid and sgid bits. - * 7) MS_NOATIME - do not update access times. + * Verify that mount(2) syscall passes for each flag setting and validate + * the flags + * + * - MS_RDONLY - mount read-only. + * - MS_NODEV - disallow access to device special files. + * - MS_NOEXEC - disallow program execution. + * - MS_SYNCHRONOUS - writes are synced at once. + * - MS_REMOUNT - alter flags of a mounted FS. + * - MS_NOSUID - ignore suid and sgid bits. + * - MS_NOATIME - do not update access times. */ #ifndef _GNU_SOURCE #define _GNU_SOURCE #endif -#include <sys/types.h> -#include <sys/mount.h> -#include <sys/stat.h> -#include <sys/wait.h> -#include <assert.h> -#include <errno.h> -#include <fcntl.h> -#include <pwd.h> -#include <unistd.h> - -#include "test.h" -#include "safe_macros.h" - -static void setup(void); -static void cleanup(void); -static int test_rwflag(int, int); - -char *TCID = "mount03"; -int TST_TOTAL = 7; - #define TEMP_FILE "temp_file" #define FILE_MODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) -#define DIR_MODE (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \ - S_IXGRP|S_IROTH|S_IXOTH) #define SUID_MODE (S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH) -static const char mntpoint[] = "mntpoint"; -static const char *device; -static const char *fs_type; -static int fildes; +#include <stdio.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <linux/limits.h> +#include <stdlib.h> +#include <pwd.h> +#include "tst_test.h" +#include "tst_safe_file_ops.h" +#include "lapi/mount.h" +#define MNTPOINT "mntpoint" +#define FILE_PATH MNTPOINT"/mount03_setuid_test" +#define TCASE_ENTRY(_flags) {.name = "Flag " #_flags, .flags = _flags} + +static int otfd; static char write_buffer[BUFSIZ]; static char read_buffer[BUFSIZ]; -static char path_name[PATH_MAX]; static char file[PATH_MAX]; - -long rwflags[] = { - MS_RDONLY, - MS_NODEV, - MS_NOEXEC, - MS_SYNCHRONOUS, - MS_RDONLY, - MS_NOSUID, - MS_NOATIME, +static char nobody_uid[] = "nobody"; +static struct passwd *ltpuser; + +static struct tcase { + char *name; + unsigned int flags; +} tcases[] = { + TCASE_ENTRY(MS_RDONLY), + TCASE_ENTRY(MS_NODEV), + TCASE_ENTRY(MS_NOEXEC), + TCASE_ENTRY(MS_SYNCHRONOUS), + TCASE_ENTRY(MS_RDONLY), + TCASE_ENTRY(MS_NOSUID), + TCASE_ENTRY(MS_NOATIME), }; -int main(int argc, char *argv[]) +static int test_ms_nosuid(void) { - int lc, i; - - tst_parse_opts(argc, argv, NULL, NULL); - - setup(); - - for (lc = 0; TEST_LOOPING(lc); lc++) { + int pid, status; - tst_count = 0; - - for (i = 0; i < TST_TOTAL; ++i) { - - TEST(mount(device, mntpoint, fs_type, rwflags[i], - NULL)); - - if (TEST_RETURN != 0) { - tst_resm(TFAIL | TTERRNO, "mount(2) failed"); - continue; - } - - /* Validate the rwflag */ - if (test_rwflag(i, lc) == 1) - tst_resm(TFAIL, "mount(2) failed while" - " validating %ld", rwflags[i]); - else - tst_resm(TPASS, "mount(2) passed with " - "rwflag = %ld", rwflags[i]); - - TEST(tst_umount(mntpoint)); - if (TEST_RETURN != 0) - tst_brkm(TBROK | TTERRNO, cleanup, - "umount(2) failed for %s", mntpoint); + switch (pid = SAFE_FORK()) { + case -1: + tst_brk(TFAIL | TERRNO, "fork failed"); + break; + case 0: + SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid); + + execlp(file, basename(file), NULL); + exit(1); + default: + waitpid(pid, &status, 0); + if (WIFEXITED(status)) { + /* reset the setup_uid */ + if (status) + return 0; } + return 1; } - - cleanup(); - tst_exit(); + return 0; } -/* - * test_rwflag(int i, int cnt) - * Validate the mount system call for rwflags. - */ -int test_rwflag(int i, int cnt) +static void test_rwflag(int i) { - int ret, fd, pid, status; - char nobody_uid[] = "nobody"; + int ret; time_t atime; - struct passwd *ltpuser; struct stat file_stat; char readbuf[20]; @@ -138,56 +99,42 @@ int test_rwflag(int i, int cnt) case 0: /* Validate MS_RDONLY flag of mount call */ - snprintf(file, PATH_MAX, "%stmp", path_name); - fd = open(file, O_CREAT | O_RDWR, S_IRWXU); - if (fd == -1) { - if (errno == EROFS) { - return 0; - } else { - tst_resm(TWARN | TERRNO, - "open didn't fail with EROFS"); - return 1; - } - } - close(fd); - return 1; + snprintf(file, PATH_MAX, "%s/tmp", MNTPOINT); + TST_EXP_FAIL2(open(file, O_CREAT | O_RDWR, S_IRWXU), + EROFS, "mount(2) passed with flag MS_RDONLY: " + "open fail with EROFS as expected"); + + otfd = TST_RET; + break; case 1: /* Validate MS_NODEV flag of mount call */ - snprintf(file, PATH_MAX, "%smynod_%d_%d", path_name, getpid(), - cnt); - if (mknod(file, S_IFBLK | 0777, 0) == 0) { - fd = open(file, O_RDWR, S_IRWXU); - if (fd == -1) { - if (errno == EACCES) { - return 0; - } else { - tst_resm(TWARN | TERRNO, - "open didn't fail with EACCES"); - return 1; - } - } - close(fd); - } else { - tst_resm(TWARN | TERRNO, "mknod(2) failed to create %s", - file); - return 1; + snprintf(file, PATH_MAX, "%s/mynod_%d", MNTPOINT, getpid()); + if (SAFE_MKNOD(file, S_IFBLK | 0777, 0) == 0) { + TST_EXP_FAIL2(open(file, O_RDWR, S_IRWXU), + EACCES, "mount(2) passed with flag MS_NODEV: " + "open fail with EACCES as expected"); + otfd = TST_RET; } - return 1; + SAFE_UNLINK(file); + break; case 2: /* Validate MS_NOEXEC flag of mount call */ - snprintf(file, PATH_MAX, "%stmp1", path_name); - fd = open(file, O_CREAT | O_RDWR, S_IRWXU); - if (fd == -1) { - tst_resm(TWARN | TERRNO, "opening %s failed", file); - } else { - close(fd); + snprintf(file, PATH_MAX, "%s/tmp1", MNTPOINT); + TST_EXP_FD_SILENT(open(file, O_CREAT | O_RDWR, S_IRWXU), + "opening %s failed", file); + otfd = TST_RET; + if (otfd >= 0) { + SAFE_CLOSE(otfd); ret = execlp(file, basename(file), NULL); - if ((ret == -1) && (errno == EACCES)) - return 0; + if ((ret == -1) && (errno == EACCES)) { + tst_res(TPASS, "mount(2) passed with flag MS_NOEXEC"); + break; + } } - return 1; + tst_brk(TFAIL | TERRNO, "mount(2) failed with flag MS_NOEXEC"); + break; case 3: /* * Validate MS_SYNCHRONOUS flag of mount call. @@ -197,193 +144,134 @@ int test_rwflag(int i, int cnt) strcpy(write_buffer, "abcdefghijklmnopqrstuvwxyz"); /* Creat a temporary file under above directory */ - snprintf(file, PATH_MAX, "%s%s", path_name, TEMP_FILE); - fildes = open(file, O_RDWR | O_CREAT, FILE_MODE); - if (fildes == -1) { - tst_resm(TWARN | TERRNO, - "open(%s, O_RDWR|O_CREAT, %#o) failed", - file, FILE_MODE); - return 1; - } + snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TEMP_FILE); + TST_EXP_FD_SILENT(open(file, O_RDWR | O_CREAT, FILE_MODE), + "open(%s, O_RDWR|O_CREAT, %#o) failed", + file, FILE_MODE); + otfd = TST_RET; /* Write the buffer data into file */ - if (write(fildes, write_buffer, strlen(write_buffer)) != - (long)strlen(write_buffer)) { - tst_resm(TWARN | TERRNO, "writing to %s failed", file); - close(fildes); - return 1; - } + SAFE_WRITE(1, otfd, write_buffer, strlen(write_buffer)); /* Set the file ptr to b'nning of file */ - if (lseek(fildes, 0, SEEK_SET) < 0) { - tst_resm(TWARN, "lseek() failed on %s, error=" - " %d", file, errno); - close(fildes); - return 1; - } + SAFE_LSEEK(otfd, 0, SEEK_SET); /* Read the contents of file */ - if (read(fildes, read_buffer, sizeof(read_buffer)) > 0) { + if (SAFE_READ(0, otfd, read_buffer, sizeof(read_buffer)) > 0) { if (strcmp(read_buffer, write_buffer)) { - tst_resm(TWARN, "Data read from %s and written " + tst_brk(TFAIL, "Data read from %s and written " "mismatch", file); - close(fildes); - return 1; } else { - close(fildes); - return 0; + SAFE_CLOSE(otfd); + tst_res(TPASS, "mount(2) passed with flag MS_SYNCHRONOUS"); + break; } } else { - tst_resm(TWARN | TERRNO, "read() Fails on %s", file); - close(fildes); - return 1; + tst_brk(TFAIL | TERRNO, "read() Fails on %s", file); } - + break; case 4: /* Validate MS_REMOUNT flag of mount call */ - TEST(mount(device, mntpoint, fs_type, MS_REMOUNT, NULL)); - if (TEST_RETURN != 0) { - tst_resm(TWARN | TTERRNO, "mount(2) failed to remount"); - return 1; + TEST(mount(tst_device->dev, MNTPOINT, tst_device->fs_type, MS_REMOUNT, NULL)); + if (TST_RET != 0) { + tst_brk(TFAIL | TTERRNO, "mount(2) failed to remount"); } else { - snprintf(file, PATH_MAX, "%stmp2", path_name); - fd = open(file, O_CREAT | O_RDWR, S_IRWXU); - if (fd == -1) { - tst_resm(TWARN, "open(%s) on readonly " + snprintf(file, PATH_MAX, "%s/tmp2", MNTPOINT); + TEST(open(file, O_CREAT | O_RDWR, S_IRWXU)); + otfd = TST_RET; + if (otfd == -1) { + tst_res(TFAIL, "open(%s) on readonly " "filesystem passed", file); - return 1; - } else { - close(fd); - return 0; - } + } else + tst_res(TPASS, "mount(2) passed with flag MS_REMOUNT"); } + break; case 5: /* Validate MS_NOSUID flag of mount call */ - snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name); - - pid = fork(); - switch (pid) { - case -1: - tst_resm(TBROK | TERRNO, "fork failed"); - return 1; - case 0: - ltpuser = getpwnam(nobody_uid); - if (setreuid(ltpuser->pw_uid, ltpuser->pw_uid) == -1) - tst_resm(TWARN | TERRNO, - "seteuid() failed to change euid to %d", - ltpuser->pw_uid); - - execlp(file, basename(file), NULL); - exit(1); - default: - waitpid(pid, &status, 0); - if (WIFEXITED(status)) { - /* reset the setup_uid */ - if (status) - return 0; - } - return 1; - } + snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT); + TEST(test_ms_nosuid()); + if (TST_RET == 0) + tst_res(TPASS, "mount(2) passed with flag MS_NOSUID"); + else + tst_res(TFAIL, "mount(2) failed with flag MS_NOSUID"); + break; case 6: /* Validate MS_NOATIME flag of mount call */ - snprintf(file, PATH_MAX, "%satime", path_name); - fd = open(file, O_CREAT | O_RDWR, S_IRWXU); - if (fd == -1) { - tst_resm(TWARN | TERRNO, "opening %s failed", file); - return 1; - } + snprintf(file, PATH_MAX, "%s/atime", MNTPOINT); + TST_EXP_FD_SILENT(open(file, O_CREAT | O_RDWR, S_IRWXU)); + otfd = TST_RET; - if (write(fd, "TEST_MS_NOATIME", 15) != 15) { - tst_resm(TWARN | TERRNO, "write %s failed", file); - close(fd); - return 1; - } + SAFE_WRITE(1, otfd, "TEST_MS_NOATIME", 15); - if (fstat(fd, &file_stat) == -1) { - tst_resm(TWARN | TERRNO, "stat %s failed #1", file); - close(fd); - return 1; - } + SAFE_FSTAT(otfd, &file_stat); atime = file_stat.st_atime; sleep(1); - if (read(fd, readbuf, sizeof(readbuf)) == -1) { - tst_resm(TWARN | TERRNO, "read %s failed", file); - close(fd); - return 1; - } + SAFE_READ(0, otfd, readbuf, sizeof(readbuf)); - if (fstat(fd, &file_stat) == -1) { - tst_resm(TWARN | TERRNO, "stat %s failed #2", file); - close(fd); - return 1; - } - close(fd); + SAFE_FSTAT(otfd, &file_stat); if (file_stat.st_atime != atime) { - tst_resm(TWARN, "access time is updated"); - return 1; + tst_res(TFAIL, "access time is updated"); + break; } - return 0; + tst_res(TPASS, "mount(2) passed with flag MS_NOATIME"); } - return 0; } -static void setup(void) +static void run(unsigned int n) { - char path[PATH_MAX]; - struct stat file_stat; - - tst_sig(FORK, DEF_HANDLER, cleanup); - - tst_require_root(); - - tst_tmpdir(); - - fs_type = tst_dev_fs_type(); - device = tst_acquire_device(cleanup); - - if (!device) - tst_brkm(TCONF, cleanup, "Failed to obtain block device"); + struct tcase *tc = &tcases[n]; - tst_mkfs(cleanup, device, fs_type, NULL, NULL); + TEST(mount(tst_device->dev, MNTPOINT, tst_device->fs_type, + tc->flags, NULL)); + test_rwflag(n); - SAFE_MKDIR(cleanup, mntpoint, DIR_MODE); - - if (getcwd(path_name, sizeof(path_name)) == NULL) - tst_brkm(TBROK, cleanup, "getcwd failed"); - - if (chmod(path_name, DIR_MODE) != 0) - tst_brkm(TBROK, cleanup, "chmod(%s, %#o) failed", - path_name, DIR_MODE); - - strncpy(path, path_name, PATH_MAX); - snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint); + if (otfd >= 0) + SAFE_CLOSE(otfd); + if (tst_is_mounted(MNTPOINT)) + SAFE_UMOUNT(MNTPOINT); +} - SAFE_MOUNT(cleanup, device, mntpoint, fs_type, 0, NULL); - TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name); +static void cleanup(void) +{ + if (otfd > -1) + SAFE_CLOSE(otfd); +} - snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name); - SAFE_STAT(cleanup, file, &file_stat); +static void setup(void) +{ + struct stat file_stat = {0}; + ltpuser = getpwnam(nobody_uid); + if (ltpuser == NULL) + tst_res(TFAIL, "getpwnam failed"); + snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT); + SAFE_STAT(MNTPOINT, &file_stat); if (file_stat.st_mode != SUID_MODE && - chmod(file, SUID_MODE) < 0) - tst_brkm(TBROK, cleanup, - "setuid for setuid_test failed"); - SAFE_UMOUNT(cleanup, mntpoint); - - TEST_PAUSE; + chmod(MNTPOINT, SUID_MODE) < 0) + tst_res(TFAIL, "setuid for setuid_test failed"); } -static void cleanup(void) -{ - if (device) - tst_release_device(device); +static const char *const resource_files[] = { + "mount03_setuid_test", + NULL, +}; - tst_rmdir(); -} +static struct tst_test test = { + .tcnt = ARRAY_SIZE(tcases), + .test = run, + .setup = setup, + .cleanup = cleanup, + .needs_root = 1, + .format_device = 1, + .resource_files = resource_files, + .forks_child = 1, + .mntpoint = MNTPOINT, + .skip_filesystems = (const char *const []){"fuse", NULL}, +};
Signed-off-by: Chen Hanxiao <chenhx.fnst@fujitsu.com> --- testcases/kernel/syscalls/mount/mount03.c | 462 ++++++++-------------- 1 file changed, 175 insertions(+), 287 deletions(-)