| Message ID | 20240306104609.17141-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] chdir01.c: set umask to 0 within setup | expand |
Hi, you're trying to fix a vfat mount quirk. We should fix that in the LTP library instead, e.g. by setting umask(0) and then restoring the original value inside safe_mount(). On 06. 03. 24 11:46, Wei Gao via ltp wrote: > When system's default umask is 0077, this will trigger following issues: > chdir01.c:100: TFAIL: nobody: chdir("subdir") returned unexpected value -1: EACCES (13) > > Signed-off-by: Wei Gao <wegao@suse.com> > --- > testcases/kernel/syscalls/chdir/chdir01.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/chdir/chdir01.c b/testcases/kernel/syscalls/chdir/chdir01.c > index d50a8f50c..97a707199 100644 > --- a/testcases/kernel/syscalls/chdir/chdir01.c > +++ b/testcases/kernel/syscalls/chdir/chdir01.c > @@ -56,12 +56,15 @@ static struct test_case { > > static void setup(void) > { > + mode_t old_umask = umask(0); > + > + SAFE_MKFS(tst_device->dev, tst_device->fs_type, NULL, NULL);Hi, > + SAFE_MOUNT(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, 0); > + > char *cwd; > int fd; > struct stat statbuf; > > - umask(0); > - > cwd = SAFE_GETCWD(NULL, 0); > workdir = SAFE_MALLOC(strlen(cwd) + strlen(MNTPOINT) + 2); > sprintf(workdir, "%s/%s", cwd, MNTPOINT); > @@ -89,6 +92,7 @@ static void setup(void) > > if (!ltpuser) > ltpuser = SAFE_GETPWNAM(TESTUSER); > + umask(old_umask); > } > > static void check_result(const char *user, const char *name, int retval, > @@ -146,13 +150,14 @@ static void cleanup(void) > { > SAFE_CHDIR(".."); > free(workdir); > + SAFE_UMOUNT(MNTPOINT); > } > > static struct tst_test test = { > .needs_root = 1, > - .mount_device = 1, > .mntpoint = MNTPOINT, > .all_filesystems = 1, > + .needs_device = 1, > .test = run, > .tcnt = ARRAY_SIZE(testcase_list), > .setup = setup,
Hi Martin, all, > Hi, > you're trying to fix a vfat mount quirk. We should fix that in the LTP > library instead, e.g. by setting umask(0) and then restoring the original > value inside safe_mount(). This makes sense. FYI Wei first tried to adjust umask globally for all tests in the do_setup() [1], which I worried it would influence tests. Later Li fixed problem in cgroup tests [2]. This is obviously more general solution, Wei please send a patch for it and to the commit message Suggested-by: Martin Doucha <mdoucha@suse.cz> While we are fixing issues caused by too restrictive umask (Wei fixed e.g. statx07 [3]), just to let you know that some failures are kernel failures (at least creat09 which uses all_filesystems, had bug on XFS [4], which got fixed in the kernel). Kind regards, Petr [1] https://lore.kernel.org/ltp/20240219134845.22171-1-wegao@suse.com/ [2] https://github.com/linux-test-project/ltp/commit/50626b4a1d01caacd418156ec997853bd4a9fc39 [3] https://github.com/linux-test-project/ltp/commit/d95f453ac624dc159d3acddb62eadeb9a8215f0e [4] https://lore.kernel.org/ltp/62343BF2.1020006@fujitsu.com/ > On 06. 03. 24 11:46, Wei Gao via ltp wrote: > > When system's default umask is 0077, this will trigger following issues: > > chdir01.c:100: TFAIL: nobody: chdir("subdir") returned unexpected value -1: EACCES (13) > > Signed-off-by: Wei Gao <wegao@suse.com> > > --- > > testcases/kernel/syscalls/chdir/chdir01.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/chdir/chdir01.c b/testcases/kernel/syscalls/chdir/chdir01.c > > index d50a8f50c..97a707199 100644 > > --- a/testcases/kernel/syscalls/chdir/chdir01.c > > +++ b/testcases/kernel/syscalls/chdir/chdir01.c > > @@ -56,12 +56,15 @@ static struct test_case { > > static void setup(void) > > { > > + mode_t old_umask = umask(0); > > + > > + SAFE_MKFS(tst_device->dev, tst_device->fs_type, NULL, NULL);Hi, > > + SAFE_MOUNT(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, 0); > > + > > char *cwd; > > int fd; > > struct stat statbuf; > > - umask(0); > > - > > cwd = SAFE_GETCWD(NULL, 0); > > workdir = SAFE_MALLOC(strlen(cwd) + strlen(MNTPOINT) + 2); > > sprintf(workdir, "%s/%s", cwd, MNTPOINT); > > @@ -89,6 +92,7 @@ static void setup(void) > > if (!ltpuser) > > ltpuser = SAFE_GETPWNAM(TESTUSER); > > + umask(old_umask); > > } > > static void check_result(const char *user, const char *name, int retval, > > @@ -146,13 +150,14 @@ static void cleanup(void) > > { > > SAFE_CHDIR(".."); > > free(workdir); > > + SAFE_UMOUNT(MNTPOINT); > > } > > static struct tst_test test = { > > .needs_root = 1, > > - .mount_device = 1, > > .mntpoint = MNTPOINT, > > .all_filesystems = 1, > > + .needs_device = 1, > > .test = run, > > .tcnt = ARRAY_SIZE(testcase_list), > > .setup = setup,
> Hi Martin, all, > > Hi, > > you're trying to fix a vfat mount quirk. We should fix that in the LTP > > library instead, e.g. by setting umask(0) and then restoring the original > > value inside safe_mount(). > This makes sense. FYI Wei first tried to adjust umask globally for all tests in > the do_setup() [1], which I worried it would influence tests. Also, it would be good to update: https://github.com/linux-test-project/ltp/wiki/C-Test-API#21-umask Kind regards, Petr > Later Li fixed problem in cgroup tests [2]. This is obviously more general > solution, Wei please send a patch for it and to the commit message > Suggested-by: Martin Doucha <mdoucha@suse.cz> > While we are fixing issues caused by too restrictive umask (Wei fixed e.g. > statx07 [3]), just to let you know that some failures are kernel failures (at > least creat09 which uses all_filesystems, had bug on XFS [4], which got fixed > in the kernel). > Kind regards, > Petr > [1] https://lore.kernel.org/ltp/20240219134845.22171-1-wegao@suse.com/ > [2] https://github.com/linux-test-project/ltp/commit/50626b4a1d01caacd418156ec997853bd4a9fc39 > [3] https://github.com/linux-test-project/ltp/commit/d95f453ac624dc159d3acddb62eadeb9a8215f0e > [4] https://lore.kernel.org/ltp/62343BF2.1020006@fujitsu.com/
On Thu, Mar 07, 2024 at 04:18:35PM +0100, Martin Doucha wrote: > Hi, > you're trying to fix a vfat mount quirk. We should fix that in the LTP > library instead, e.g. by setting umask(0) and then restoring the original > value inside safe_mount(). Thanks for your feedback. For chdir case i just use Petr's below suggestion(Detail info you can check patch link in below): "2) tests, which set .mount_device = 1 and have more restrictive umask will not work. Workaround would be to not use it and mount manually in the setup(). Or, reset umask with umask(0)." https://patchwork.ozlabs.org/project/ltp/patch/20240219134845.22171-1-wegao@suse.com/ BTW: for similar issues we also do fix within test file in stead of lib such as: https://patchwork.ozlabs.org/project/ltp/patch/20240303103105.13401-1-wegao@suse.com/ https://patchwork.ozlabs.org/project/ltp/patch/20240301102347.3035546-1-liwang@redhat.com/ > -- > Martin Doucha mdoucha@suse.cz > SW Quality Engineer > SUSE LINUX, s.r.o. > CORSO IIa > Krizikova 148/34 > 186 00 Prague 8 > Czech Republic >
On 08. 03. 24 0:21, Wei Gao wrote: > On Thu, Mar 07, 2024 at 04:18:35PM +0100, Martin Doucha wrote: >> Hi, >> you're trying to fix a vfat mount quirk. We should fix that in the LTP >> library instead, e.g. by setting umask(0) and then restoring the original >> value inside safe_mount(). > > Thanks for your feedback. > > For chdir case i just use Petr's below suggestion(Detail info you can check patch link > in below): > > "2) tests, which set .mount_device = 1 and have more restrictive umask will not > work. Workaround would be to not use it and mount manually in the setup(). > Or, reset umask with umask(0)." > > https://patchwork.ozlabs.org/project/ltp/patch/20240219134845.22171-1-wegao@suse.com/ I'd rather avoid mounting in setup unless you need to set special mount parameters. Mount-time umask does not matter for most filesystem. The exception are vfat and exfat which don't have any internal concept of access permissions and instead you need to either pass mount options that'll define access permissions for all files and directories, otherwise current process umask value will be used as the default. So resetting umask to 0 before mount and restoring immediately after is perfectly safe. But we should later fix it properly by implementing per-filesystem mount options.
> On 08. 03. 24 0:21, Wei Gao wrote: > > On Thu, Mar 07, 2024 at 04:18:35PM +0100, Martin Doucha wrote: > > > Hi, > > > you're trying to fix a vfat mount quirk. We should fix that in the LTP > > > library instead, e.g. by setting umask(0) and then restoring the original > > > value inside safe_mount(). > > Thanks for your feedback. > > For chdir case i just use Petr's below suggestion(Detail info you can check patch link > > in below): > > "2) tests, which set .mount_device = 1 and have more restrictive umask will not > > work. Workaround would be to not use it and mount manually in the setup(). > > Or, reset umask with umask(0)." > > https://patchwork.ozlabs.org/project/ltp/patch/20240219134845.22171-1-wegao@suse.com/ > I'd rather avoid mounting in setup unless you need to set special mount > parameters. > Mount-time umask does not matter for most filesystem. The exception are vfat > and exfat which don't have any internal concept of access permissions and > instead you need to either pass mount options that'll define access > permissions for all files and directories, otherwise current process umask > value will be used as the default. +1 > So resetting umask to 0 before mount and restoring immediately after is > perfectly safe. But we should later fix it properly by implementing > per-filesystem mount options. Yeah, I was thinking about it, but haven't implement it yet because not many tests need it and it can be mostly workarounded. Do you know tests (beside this one) which need it? Kind regards, Petr
diff --git a/testcases/kernel/syscalls/chdir/chdir01.c b/testcases/kernel/syscalls/chdir/chdir01.c index d50a8f50c..97a707199 100644 --- a/testcases/kernel/syscalls/chdir/chdir01.c +++ b/testcases/kernel/syscalls/chdir/chdir01.c @@ -56,12 +56,15 @@ static struct test_case { static void setup(void) { + mode_t old_umask = umask(0); + + SAFE_MKFS(tst_device->dev, tst_device->fs_type, NULL, NULL); + SAFE_MOUNT(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, 0); + char *cwd; int fd; struct stat statbuf; - umask(0); - cwd = SAFE_GETCWD(NULL, 0); workdir = SAFE_MALLOC(strlen(cwd) + strlen(MNTPOINT) + 2); sprintf(workdir, "%s/%s", cwd, MNTPOINT); @@ -89,6 +92,7 @@ static void setup(void) if (!ltpuser) ltpuser = SAFE_GETPWNAM(TESTUSER); + umask(old_umask); } static void check_result(const char *user, const char *name, int retval, @@ -146,13 +150,14 @@ static void cleanup(void) { SAFE_CHDIR(".."); free(workdir); + SAFE_UMOUNT(MNTPOINT); } static struct tst_test test = { .needs_root = 1, - .mount_device = 1, .mntpoint = MNTPOINT, .all_filesystems = 1, + .needs_device = 1, .test = run, .tcnt = ARRAY_SIZE(testcase_list), .setup = setup,
When system's default umask is 0077, this will trigger following issues: chdir01.c:100: TFAIL: nobody: chdir("subdir") returned unexpected value -1: EACCES (13) Signed-off-by: Wei Gao <wegao@suse.com> --- testcases/kernel/syscalls/chdir/chdir01.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)