diff mbox series

[RFC,v1,1/2] safe_macros.c: Fix missing ro flag for FUSE NTFS mounts

Message ID 20250513165640.185122-2-japo@linux.ibm.com
State Needs Review / ACK
Headers show
Series Fix NTFS-related failures in statmount02 and | expand

Checks

Context Check Description
ltpci/debian_stable_aarch64-linux-gnu-gcc_arm64 success success
ltpci/debian_stable_s390x-linux-gnu-gcc_s390x success success
ltpci/debian_stable_gcc fail failure
ltpci/debian_stable_powerpc64le-linux-gnu-gcc_ppc64el success success
ltpci/ubuntu_jammy_gcc fail failure
ltpci/debian_stable_gcc fail failure
ltpci/ubuntu_bionic_gcc fail failure
ltpci/debian_testing_gcc fail failure
ltpci/alpine_latest_gcc fail failure
ltpci/opensuse-leap_latest_gcc fail failure
ltpci/quay-io-centos-centos_stream9_gcc fail failure
ltpci/opensuse-archive_42-2_gcc fail failure
ltpci/debian_testing_clang fail failure
ltpci/debian_oldstable_gcc fail failure
ltpci/debian_oldstable_clang fail failure
ltpci/fedora_latest_clang fail failure

Commit Message

Jan Polensky May 13, 2025, 4:56 p.m. UTC 
The test incorrectly assumes that NTFS mounts are read-only, but the mount
command does not explicitly set the read-only flag. As a result, the test fails
when checking `sb_flags` against `MS_RDONLY`.

Old behavior:

	sudo LTP_SINGLE_FS_TYPE=ntfs strace -e trace=mount,statmount -o log.log -s 128 -f ./statmount02
	...
	statmount02.c:47: TFAIL: st_mount->sb_flags (0) != MS_RDONLY (1)
	...

Relevant log excerpt:

	3890601 mount("/dev/zero", "/tmp/mountBDSEqk", "ntfs", 0, NULL) = -1 ENOTBLK (Block device required)
	3890608 mount("/dev/loop0", "/tmp/LTP_staTPRruR/mntpoint", "fuseblk", 0, "allow_other,blksize=4096,fd=4,rootmode=40000,user_id=0,group_id=0") = 0
	3890607 statmount({size=24, mnt_id=0x80010957, param=STATMOUNT_SB_BASIC}, {size=512, mask=STATMOUNT_SB_BASIC, sb_dev_major=7, sb_dev_minor=0, sb_magic=FUSE_SUPER_MAGIC, sb_flags=0}, 512, 0) = 0

Signed-off-by: Jan Polensky <japo@linux.ibm.com>
---
 lib/safe_macros.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

--
2.49.0

Comments

Petr Vorel May 15, 2025, 7:48 p.m. UTC | #1 
Hi Jan,

> The test incorrectly assumes that NTFS mounts are read-only, but the mount
> command does not explicitly set the read-only flag. As a result, the test fails
> when checking `sb_flags` against `MS_RDONLY`.

> Old behavior:

Reviewed-by: Petr Vorel <pvorel@suse.cz>
IMHO candidate for a release. Li, Cyril WDYT?

> 	sudo LTP_SINGLE_FS_TYPE=ntfs strace -e trace=mount,statmount -o log.log -s 128 -f ./statmount02
FYI the problem is currently LTP_SINGLE_FS_TYPE is still meant only for testing
one of the filesystems among .all_filesystems => these in fs_type_whitelist[]
(see lib/tst_supported_fs_types.c).

BTW that's similar to use case we haven't even formulated in issue:
https://lore.kernel.org/ltp/Z066Fj9VQVlTOMp_@rei.lan/
https://lore.kernel.org/ltp/Z1mA2wzjW0hpQxUH@yuki.lan/
(The only difference is that this requires to use filesystem not in
fs_type_whitelist[], therefore minor bug for it was found).

Kind regards,
Petr

> 	...
> 	statmount02.c:47: TFAIL: st_mount->sb_flags (0) != MS_RDONLY (1)
> 	...

> Relevant log excerpt:

> 	3890601 mount("/dev/zero", "/tmp/mountBDSEqk", "ntfs", 0, NULL) = -1 ENOTBLK (Block device required)
> 	3890608 mount("/dev/loop0", "/tmp/LTP_staTPRruR/mntpoint", "fuseblk", 0, "allow_other,blksize=4096,fd=4,rootmode=40000,user_id=0,group_id=0") = 0
> 	3890607 statmount({size=24, mnt_id=0x80010957, param=STATMOUNT_SB_BASIC}, {size=512, mask=STATMOUNT_SB_BASIC, sb_dev_major=7, sb_dev_minor=0, sb_magic=FUSE_SUPER_MAGIC, sb_flags=0}, 512, 0) = 0

> Signed-off-by: Jan Polensky <japo@linux.ibm.com>
> ---
>  lib/safe_macros.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

> diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> index 6946cc5bcb94..1270b17af8f4 100644
> --- a/lib/safe_macros.c
> +++ b/lib/safe_macros.c
> @@ -942,10 +942,15 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
>  	 */
>  	if (possibly_fuse(filesystemtype)) {
>  		char buf[1024];
> +		const char* mount_fmt;

>  		tst_resm_(file, lineno, TINFO, "Trying FUSE...");
> -		snprintf(buf, sizeof(buf), "mount.%s '%s' '%s'",
> -			filesystemtype, source, target);
> +		if (!strcmp(filesystemtype, "ntfs") && mountflags & MS_RDONLY)
> +			mount_fmt = "mount.%s -o ro '%s' '%s'";
> +		else
> +			mount_fmt = "mount.%s '%s' '%s'";
> +		snprintf(buf, sizeof(buf), mount_fmt, filesystemtype,
> +				source, target);

>  		rval = tst_system(buf);
>  		if (WIFEXITED(rval) && WEXITSTATUS(rval) == 0)
Li Wang May 16, 2025, 6:26 a.m. UTC | #2 
Petr Vorel <pvorel@suse.cz> wrote:

> Hi Jan,
>
> > The test incorrectly assumes that NTFS mounts are read-only, but the mount
> > command does not explicitly set the read-only flag. As a result, the test fails
> > when checking `sb_flags` against `MS_RDONLY`.
>
> > Old behavior:
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> IMHO candidate for a release. Li, Cyril WDYT?

I agree.
Cyril Hrubis May 21, 2025, 9:37 a.m. UTC | #3 
Hi!
> diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> index 6946cc5bcb94..1270b17af8f4 100644
> --- a/lib/safe_macros.c
> +++ b/lib/safe_macros.c
> @@ -942,10 +942,15 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
>  	 */
>  	if (possibly_fuse(filesystemtype)) {
>  		char buf[1024];
> +		const char* mount_fmt;
> 
>  		tst_resm_(file, lineno, TINFO, "Trying FUSE...");
> -		snprintf(buf, sizeof(buf), "mount.%s '%s' '%s'",
> -			filesystemtype, source, target);
> +		if (!strcmp(filesystemtype, "ntfs") && mountflags & MS_RDONLY)

Do we need to limit this to "ntfs"? I suppose that for other FUSE
filesystems would have the same problem, e.g. exfat.

Other than this the patch looks good.
Cyril Hrubis May 21, 2025, 9:55 a.m. UTC | #4 
Hi!
> > diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> > index 6946cc5bcb94..1270b17af8f4 100644
> > --- a/lib/safe_macros.c
> > +++ b/lib/safe_macros.c
> > @@ -942,10 +942,15 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
> >  	 */
> >  	if (possibly_fuse(filesystemtype)) {
> >  		char buf[1024];
> > +		const char* mount_fmt;
> > 
> >  		tst_resm_(file, lineno, TINFO, "Trying FUSE...");
> > -		snprintf(buf, sizeof(buf), "mount.%s '%s' '%s'",
> > -			filesystemtype, source, target);
> > +		if (!strcmp(filesystemtype, "ntfs") && mountflags & MS_RDONLY)
> 
> Do we need to limit this to "ntfs"? I suppose that for other FUSE
> filesystems would have the same problem, e.g. exfat.
> 
> Other than this the patch looks good.

Also if my patch that fixes the fuse blacklist gets applied we need to
remove the fuse from the blacklist from statmount02 to get the test
enabled after this fix.
Cyril Hrubis June 4, 2025, 11:59 a.m. UTC | #5 
Hi!
> > > --- a/lib/safe_macros.c
> > > +++ b/lib/safe_macros.c
> > > @@ -942,10 +942,15 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
> > >  	 */
> > >  	if (possibly_fuse(filesystemtype)) {
> > >  		char buf[1024];
> > > +		const char* mount_fmt;
> > > 
> > >  		tst_resm_(file, lineno, TINFO, "Trying FUSE...");
> > > -		snprintf(buf, sizeof(buf), "mount.%s '%s' '%s'",
> > > -			filesystemtype, source, target);
> > > +		if (!strcmp(filesystemtype, "ntfs") && mountflags & MS_RDONLY)
> > 
> > Do we need to limit this to "ntfs"? I suppose that for other FUSE
> > filesystems would have the same problem, e.g. exfat.
> > 
> > Other than this the patch looks good.
> 
> Also if my patch that fixes the fuse blacklist gets applied we need to
> remove the fuse from the blacklist from statmount02 to get the test
> enabled after this fix.

Ping. Are you going to work on this? I think that it's a good idea to
fix the fuse mount to apply the flags correctly and then enable the
tests that were previously disabled by this.
diff mbox series

Patch

diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index 6946cc5bcb94..1270b17af8f4 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -942,10 +942,15 @@  int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
 	 */
 	if (possibly_fuse(filesystemtype)) {
 		char buf[1024];
+		const char* mount_fmt;

 		tst_resm_(file, lineno, TINFO, "Trying FUSE...");
-		snprintf(buf, sizeof(buf), "mount.%s '%s' '%s'",
-			filesystemtype, source, target);
+		if (!strcmp(filesystemtype, "ntfs") && mountflags & MS_RDONLY)
+			mount_fmt = "mount.%s -o ro '%s' '%s'";
+		else
+			mount_fmt = "mount.%s '%s' '%s'";
+		snprintf(buf, sizeof(buf), mount_fmt, filesystemtype,
+				source, target);

 		rval = tst_system(buf);
 		if (WIFEXITED(rval) && WEXITSTATUS(rval) == 0)