| Message ID | 20240308083220.19332-1-wegao@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] safe_macros.c: set umask to 0 within safe_mount | expand |
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; > }
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; > }
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
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; }
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(-)