[LTP] [PATCH v1] chdir01.c: set umask to 0 within setup

[LTP] [PATCH v1] chdir01.c: set umask to 0 within setup

[Wei Gao via ltp]

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);
+	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,
-- 
2.35.3


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

Re: [LTP] [PATCH v1] chdir01.c: set umask to 0 within setup

[Martin Doucha]

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,

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v1] chdir01.c: set umask to 0 within setup

[Petr Vorel]

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,

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

Re: [LTP] [PATCH v1] chdir01.c: set umask to 0 within setup

[Petr Vorel]

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

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

Re: [LTP] [PATCH v1] chdir01.c: set umask to 0 within setup

[Wei Gao via ltp]

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
> 

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

Re: [LTP] [PATCH v1] chdir01.c: set umask to 0 within setup

[Martin Doucha]

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.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v1] chdir01.c: set umask to 0 within setup

[Petr Vorel]

> 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

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

[LTP] [PATCH v2] safe_macros.c: set umask to 0 within safe_mount

[Wei Gao via ltp]

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)

Suggested-by: Martin Doucha <mdoucha@suse.cz>
Signed-off-by: Wei Gao <wegao@suse.com>
---
 doc/C-Test-API.asciidoc | 4 +++-
 lib/safe_macros.c       | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
index 08a76c403..81067b12b 100644
--- a/doc/C-Test-API.asciidoc
+++ b/doc/C-Test-API.asciidoc
@@ -2460,7 +2460,9 @@ with 'open()' or 'creat()' etc, the mode specified as the last parameter *is
 not* the mode the file is created with. The mode depends on current 'umask()'
 settings which may clear some of the bits. If your test depends on specific
 file permissions you need either to change umask to 0 or 'chmod()' the file
-afterwards or use 'SAFE_TOUCH()' that does the 'chmod()' for you.
+afterwards or use 'SAFE_TOUCH()' that does the 'chmod()' for you. SAFE_MOUNT
+also does similar action such as setting umask(0) and then restoring the
+original value.
 
 2.2 access()
 ~~~~~~~~~~~~
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index 951e1b064..109268587 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -913,7 +913,10 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
 	 * the kernel's NTFS driver doesn't have proper write support.
 	 */
 	if (!filesystemtype || strcmp(filesystemtype, "ntfs")) {
+		mode_t old_umask = umask(0);
+
 		rval = mount(source, target, filesystemtype, mountflags, data);
+		umask(old_umask);
 		if (!rval)
 			return 0;
 	}
-- 
2.35.3


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

Re: [LTP] [PATCH v2] safe_macros.c: set umask to 0 within safe_mount

[Petr Vorel]

Hi Wei, all,

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

> Suggested-by: Martin Doucha <mdoucha@suse.cz>
> Signed-off-by: Wei Gao <wegao@suse.com>

> ---
>  doc/C-Test-API.asciidoc | 4 +++-
>  lib/safe_macros.c       | 3 +++
>  2 files changed, 6 insertions(+), 1 deletion(-)

> diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
> index 08a76c403..81067b12b 100644
> --- a/doc/C-Test-API.asciidoc
> +++ b/doc/C-Test-API.asciidoc
> @@ -2460,7 +2460,9 @@ with 'open()' or 'creat()' etc, the mode specified as the last parameter *is
>  not* the mode the file is created with. The mode depends on current 'umask()'
>  settings which may clear some of the bits. If your test depends on specific
>  file permissions you need either to change umask to 0 or 'chmod()' the file
> -afterwards or use 'SAFE_TOUCH()' that does the 'chmod()' for you.
> +afterwards or use 'SAFE_TOUCH()' that does the 'chmod()' for you. SAFE_MOUNT
s/SAFE_MOUNT/'SAFE_MOUNT()'/

> +also does similar action such as setting umask(0) and then restoring the
> +original value.

I'm not sure about the wording. Maybe:

Temporarily clearing umask with 'umask(0)' is done before mounting in
'SAFE_MOUNT()' and before creating a new subdir in the cgroup in 'cgroup_dir_mk()'.

(based on my patch
https://lore.kernel.org/ltp/20240307232511.228396-1-pvorel@suse.cz/).

It could be changed before merge.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

>  2.2 access()
>  ~~~~~~~~~~~~
> diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> index 951e1b064..109268587 100644
> --- a/lib/safe_macros.c
> +++ b/lib/safe_macros.c
> @@ -913,7 +913,10 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
>  	 * the kernel's NTFS driver doesn't have proper write support.
>  	 */
>  	if (!filesystemtype || strcmp(filesystemtype, "ntfs")) {
> +		mode_t old_umask = umask(0);
> +
>  		rval = mount(source, target, filesystemtype, mountflags, data);
> +		umask(old_umask);
>  		if (!rval)
>  			return 0;
>  	}

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

Re: [LTP] [PATCH v2] safe_macros.c: set umask to 0 within safe_mount

[Martin Doucha]

Hi,
the doc change is not needed. For the lib change alone:

Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 08. 03. 24 9:32, Wei Gao 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)
> 
> Suggested-by: Martin Doucha <mdoucha@suse.cz>
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>   doc/C-Test-API.asciidoc | 4 +++-
>   lib/safe_macros.c       | 3 +++
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
> index 08a76c403..81067b12b 100644
> --- a/doc/C-Test-API.asciidoc
> +++ b/doc/C-Test-API.asciidoc
> @@ -2460,7 +2460,9 @@ with 'open()' or 'creat()' etc, the mode specified as the last parameter *is
>   not* the mode the file is created with. The mode depends on current 'umask()'
>   settings which may clear some of the bits. If your test depends on specific
>   file permissions you need either to change umask to 0 or 'chmod()' the file
> -afterwards or use 'SAFE_TOUCH()' that does the 'chmod()' for you.
> +afterwards or use 'SAFE_TOUCH()' that does the 'chmod()' for you. SAFE_MOUNT
> +also does similar action such as setting umask(0) and then restoring the
> +original value.
>   
>   2.2 access()
>   ~~~~~~~~~~~~
> diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> index 951e1b064..109268587 100644
> --- a/lib/safe_macros.c
> +++ b/lib/safe_macros.c
> @@ -913,7 +913,10 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
>   	 * the kernel's NTFS driver doesn't have proper write support.
>   	 */
>   	if (!filesystemtype || strcmp(filesystemtype, "ntfs")) {
> +		mode_t old_umask = umask(0);
> +
>   		rval = mount(source, target, filesystemtype, mountflags, data);
> +		umask(old_umask);
>   		if (!rval)
>   			return 0;
>   	}

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2] safe_macros.c: set umask to 0 within safe_mount

[Petr Vorel]

Hi Martin, Wei,

> Hi,
> the doc change is not needed. For the lib change alone:

+1. Merged only change in the library, reworded commit message.

Thanks to both!

Kind regards,
Petr

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