[PATCH 0/3] fs/9p: fix mmap regression

[PATCH 0/3] fs/9p: fix mmap regression

[Eric Van Hensbergen]

This series attempts to fix a reported exception with mmap
on newer kernels. 

-- original regression report --

TL;DR: mmap() seems to be broken on 9pfs on Linux 6.4. setting
"rootflags=ignoreqv" fixes it as well, but it feels like a regression.

I'm tracking down an issue which recently turned up in DistroKit [1] (an
embedded Linux distro based on the ptxdist build system). The issue was a bit
uggly, as my CI didn't find it (systems boot up normally after a while, and I
only use 9p for virtual qemu machines, while most of the test farm is real
hardware).

The qemu machine in question is qemu-system-arm, emulating an ARM v7a machine.

When starting the systems interactively, I get a lot of error output from
ldconfig, like this:

[   17.412964] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libgcc_s.so.1.
[   17.418851] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libstdc++.so.
[   17.425009] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libstdc++.so.6.0.30.
[   17.436671] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libstdc++.so.6.
[   17.448451] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libatomic.so.
[   17.456418] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libatomic.so.1.2.0.
...

Running ldconfig with strace shows this, for all libraries::

| statx(AT_FDCWD, "/lib/libnm.so.0", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW|AT_NO_AUTOMOUNT, STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFLNK|0777, stx_size=14, ...}) = 0
| statx(AT_FDCWD, "/lib/libnm.so.0", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT, STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|064 4, stx_size=862228, ...}) = 0
| openat(AT_FDCWD, "/lib/libnm.so.0", O_RDONLY|O_LARGEFILE) = 4
| statx(4, "", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT|AT_EMPTY_PATH, STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|0644, stx_s ize=862228, ...}) = 0
| mmap2(NULL, 862228, PROT_READ, MAP_SHARED, 4, 0) = -1 ENODEV (No such device)
| write(2, "ldconfig: ", 10ldconfig: )              = 10
| write(2, "Cannot mmap file /lib/libnm.so.0"..., 34Cannot mmap file /lib/libnm.so.0.) = 34
| write(2, "\n", 1)                       = 1
| close(4)                                = 0

I could track down the breakage to

  1543b4c5071c54d76aad7a7a26a6e43082269b0c

My test setup has, in addition to the patch above, the following patches also
reverted on top of a vanilla 6.4 kernel:

  4eb3117888a923f6b9b1ad2dd093641c49a63ae5
  21e26d5e54ab7cfe6b488fd27d4d70956d07e03b

as 1543b cannot be reverted without those; however, the effect only goes away
when I also revert 1543b. The kernel has no other patches applied, only these
three reverts.

-- end bug report --

Reported-by: Robert Schwebel <r.schwebel@pengutronix.de>
Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
---
Eric Van Hensbergen (3):
      fs/9p: remove unecessary and overrestrictive check
      fs/9p: fix typo in comparison logic for cache mode
      fs/9p: fix type mismatch in file cache mode helper

 fs/9p/fid.h      | 6 +++---
 fs/9p/vfs_file.c | 2 --
 2 files changed, 3 insertions(+), 5 deletions(-)
---
base-commit: 95f41d87810083d8b3dedcce46a4e356cf4a9673
change-id: 20230716-fixes-overly-restrictive-mmap-30a23501e787

Best regards,
-- 
Eric Van Hensbergen <ericvh@kernel.org>

[PATCH 1/3] fs/9p: remove unecessary and overrestrictive check

[Eric Van Hensbergen]

This eliminates a check for shared that was overrestrictive and
duplicated a check in generic_file_readonly_mmap.

Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
---
 fs/9p/vfs_file.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index 2996fb00387fa..bda3abd6646b8 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -506,8 +506,6 @@ v9fs_file_mmap(struct file *filp, struct vm_area_struct *vma)
 
 	if (!(v9ses->cache & CACHE_WRITEBACK)) {
 		p9_debug(P9_DEBUG_CACHE, "(no mmap mode)");
-		if (vma->vm_flags & VM_MAYSHARE)
-			return -ENODEV;
 		invalidate_inode_pages2(filp->f_mapping);
 		return generic_file_readonly_mmap(filp, vma);
 	}

-- 
2.39.2

Re: [PATCH 1/3] fs/9p: remove unecessary and overrestrictive check

[Dominique Martinet]

Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:00PM +0000:
> This eliminates a check for shared that was overrestrictive and
> duplicated a check in generic_file_readonly_mmap.

generic_file_readonly_mmap checks for VM_SHARED + VM_MAYWRITE,
so it isn't exactly "duplicating" the check.. But I agree we don't need
it; we used to have the mmap op be generic_file_readonly_mmap directly
previously.

I'd argue we don't need to invalidate inode pages either if there is no
writeback cache, there shouldn't be anything in it? but that can be done
separately, and extra invalidation won't bring harm anyway.

Reviewed-by: Dominique Martinet <asmadeus@codewreck.org>

> 
> Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
> ---
>  fs/9p/vfs_file.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 2996fb00387fa..bda3abd6646b8 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -506,8 +506,6 @@ v9fs_file_mmap(struct file *filp, struct vm_area_struct *vma)
>  
>  	if (!(v9ses->cache & CACHE_WRITEBACK)) {
>  		p9_debug(P9_DEBUG_CACHE, "(no mmap mode)");
> -		if (vma->vm_flags & VM_MAYSHARE)
> -			return -ENODEV;
>  		invalidate_inode_pages2(filp->f_mapping);
>  		return generic_file_readonly_mmap(filp, vma);
>  	}
> 

-- 
Dominique Martinet | Asmadeus

Re: [PATCH 1/3] fs/9p: remove unecessary and overrestrictive check

[Christian Schoenebeck]

On Tuesday, July 18, 2023 4:45:25 AM CEST Dominique Martinet wrote:
> Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:00PM +0000:
> > This eliminates a check for shared that was overrestrictive and
> > duplicated a check in generic_file_readonly_mmap.
> 
> generic_file_readonly_mmap checks for VM_SHARED + VM_MAYWRITE,
> so it isn't exactly "duplicating" the check.. But I agree we don't need
> it; we used to have the mmap op be generic_file_readonly_mmap directly
> previously.

It should be removed from the commit log then though.

> I'd argue we don't need to invalidate inode pages either if there is no
> writeback cache, there shouldn't be anything in it? but that can be done
> separately, and extra invalidation won't bring harm anyway.

Unnecessary performance penalty? So I would drop that in a separate patch.

> Reviewed-by: Dominique Martinet <asmadeus@codewreck.org>

Feel free to add my RB as well:

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

> > 
> > Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
> > ---
> >  fs/9p/vfs_file.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> > index 2996fb00387fa..bda3abd6646b8 100644
> > --- a/fs/9p/vfs_file.c
> > +++ b/fs/9p/vfs_file.c
> > @@ -506,8 +506,6 @@ v9fs_file_mmap(struct file *filp, struct vm_area_struct *vma)
> >  
> >  	if (!(v9ses->cache & CACHE_WRITEBACK)) {
> >  		p9_debug(P9_DEBUG_CACHE, "(no mmap mode)");
> > -		if (vma->vm_flags & VM_MAYSHARE)
> > -			return -ENODEV;
> >  		invalidate_inode_pages2(filp->f_mapping);
> >  		return generic_file_readonly_mmap(filp, vma);
> >  	}
> > 
> 
> 

[PATCH 2/3] fs/9p: fix typo in comparison logic for cache mode

[Eric Van Hensbergen]

There appears to be a typo in the comparison statement for the logic
which sets a file's cache mode based on mount flags.

Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
---
 fs/9p/fid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/9p/fid.h b/fs/9p/fid.h
index 0c51889a60b33..297c2c377e3dd 100644
--- a/fs/9p/fid.h
+++ b/fs/9p/fid.h
@@ -57,7 +57,7 @@ static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
 	   (s_flags & V9FS_DIRECT_IO) || (f_flags & O_DIRECT)) {
 		fid->mode |= P9L_DIRECT; /* no read or write cache */
 	} else if ((!(s_cache & CACHE_WRITEBACK)) ||
-				(f_flags & O_DSYNC) | (s_flags & V9FS_SYNC)) {
+				(f_flags & O_DSYNC) || (s_flags & V9FS_SYNC)) {
 		fid->mode |= P9L_NOWRITECACHE;
 	}
 }

-- 
2.39.2

Re: [PATCH 2/3] fs/9p: fix typo in comparison logic for cache mode

[Dominique Martinet]

Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:01PM +0000:
> There appears to be a typo in the comparison statement for the logic
> which sets a file's cache mode based on mount flags.

Shouldn't break anything, but good fix nevertheless, thanks!

Reviewed-by: Dominique Martinet <asmadeus@codewreck.org>


> 
> Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
> ---
>  fs/9p/fid.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> index 0c51889a60b33..297c2c377e3dd 100644
> --- a/fs/9p/fid.h
> +++ b/fs/9p/fid.h
> @@ -57,7 +57,7 @@ static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
>  	   (s_flags & V9FS_DIRECT_IO) || (f_flags & O_DIRECT)) {
>  		fid->mode |= P9L_DIRECT; /* no read or write cache */
>  	} else if ((!(s_cache & CACHE_WRITEBACK)) ||
> -				(f_flags & O_DSYNC) | (s_flags & V9FS_SYNC)) {
> +				(f_flags & O_DSYNC) || (s_flags & V9FS_SYNC)) {
>  		fid->mode |= P9L_NOWRITECACHE;
>  	}
>  }
> 

-- 
Dominique Martinet | Asmadeus

Re: [PATCH 2/3] fs/9p: fix typo in comparison logic for cache mode

[Christian Schoenebeck]

On Tuesday, July 18, 2023 4:48:53 AM CEST Dominique Martinet wrote:
> Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:01PM +0000:
> > There appears to be a typo in the comparison statement for the logic
> > which sets a file's cache mode based on mount flags.
> 
> Shouldn't break anything, but good fix nevertheless, thanks!
> 
> Reviewed-by: Dominique Martinet <asmadeus@codewreck.org>

Right, at least AFAICS I would not expect any visible behaviour change by this
patch at all. So this patch is probably just a formal fix.

BTW there are a bunch of unnecessary braces in this function:

  (!s_cache) -> !s_cache

  (fid->qid.version == 0) -> fid->qid.version == 0

  (!(s_cache & CACHE_WRITEBACK)) -> !(s_cache & CACHE_WRITEBACK)

These could be wiped in a separate patch as well. Anyway ...

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

> > 
> > Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
> > ---
> >  fs/9p/fid.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> > index 0c51889a60b33..297c2c377e3dd 100644
> > --- a/fs/9p/fid.h
> > +++ b/fs/9p/fid.h
> > @@ -57,7 +57,7 @@ static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
> >  	   (s_flags & V9FS_DIRECT_IO) || (f_flags & O_DIRECT)) {
> >  		fid->mode |= P9L_DIRECT; /* no read or write cache */
> >  	} else if ((!(s_cache & CACHE_WRITEBACK)) ||
> > -				(f_flags & O_DSYNC) | (s_flags & V9FS_SYNC)) {
> > +				(f_flags & O_DSYNC) || (s_flags & V9FS_SYNC)) {
> >  		fid->mode |= P9L_NOWRITECACHE;
> >  	}
> >  }
> > 
> 
> 

[PATCH 3/3] fs/9p: fix type mismatch in file cache mode helper

[Eric Van Hensbergen]

There were two flags which had incorrect type in the
paramaters of the file cache mode helper function.

Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
---
 fs/9p/fid.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/9p/fid.h b/fs/9p/fid.h
index 297c2c377e3dd..29281b7c38870 100644
--- a/fs/9p/fid.h
+++ b/fs/9p/fid.h
@@ -46,8 +46,8 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)
  * NOTE: these are set after open so only reflect 9p client not
  * underlying file system on server.
  */
-static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
-	int s_cache, unsigned int f_flags)
+static inline void v9fs_fid_add_modes(struct p9_fid *fid, unsigned int s_flags,
+	unsigned int s_cache, unsigned int f_flags)
 {
 	if (fid->qid.type != P9_QTFILE)
 		return;

-- 
2.39.2

Re: [PATCH 3/3] fs/9p: fix type mismatch in file cache mode helper

[Dominique Martinet]

Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:02PM +0000:
> There were two flags which had incorrect type in the
> paramaters of the file cache mode helper function.

Checked the parameters passed (v9ses->cache, file->f_flags and a flags
parent function parameters) were all correctly unsigned:

Reviewed-by: Dominique Martinet <asmadeus@codewreck.org>

> 
> Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
> ---
>  fs/9p/fid.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> index 297c2c377e3dd..29281b7c38870 100644
> --- a/fs/9p/fid.h
> +++ b/fs/9p/fid.h
> @@ -46,8 +46,8 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)
>   * NOTE: these are set after open so only reflect 9p client not
>   * underlying file system on server.
>   */
> -static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
> -	int s_cache, unsigned int f_flags)
> +static inline void v9fs_fid_add_modes(struct p9_fid *fid, unsigned int s_flags,
> +	unsigned int s_cache, unsigned int f_flags)
>  {
>  	if (fid->qid.type != P9_QTFILE)
>  		return;
> 

-- 
Dominique Martinet | Asmadeus

Re: [PATCH 3/3] fs/9p: fix type mismatch in file cache mode helper

[Christian Schoenebeck]

On Monday, July 17, 2023 6:29:02 PM CEST Eric Van Hensbergen wrote:
> There were two flags which had incorrect type in the
> paramaters of the file cache mode helper function.

1. typo: "parameters"

2. I would be more precise and actually name the two flags in question in the
commit log.

> Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
> ---
>  fs/9p/fid.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> index 297c2c377e3dd..29281b7c38870 100644
> --- a/fs/9p/fid.h
> +++ b/fs/9p/fid.h
> @@ -46,8 +46,8 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)
>   * NOTE: these are set after open so only reflect 9p client not
>   * underlying file system on server.
>   */
> -static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
> -	int s_cache, unsigned int f_flags)
> +static inline void v9fs_fid_add_modes(struct p9_fid *fid, unsigned int s_flags,
> +	unsigned int s_cache, unsigned int f_flags)
>  {
>  	if (fid->qid.type != P9_QTFILE)
>  		return;
> 
> 

Re: [PATCH 0/3] fs/9p: fix mmap regression

[Thorsten Leemhuis]

On 17.07.23 18:28, Eric Van Hensbergen wrote:
> This series attempts to fix a reported exception with mmap
> on newer kernels. 
> 
> -- original regression report --
> 
> TL;DR: mmap() seems to be broken on 9pfs on Linux 6.4. setting
> "rootflags=ignoreqv" fixes it as well, but it feels like a regression.
>
> [...]
> 
> I could track down the breakage to
> 
>   1543b4c5071c54d76aad7a7a26a6e43082269b0c
> 
> My test setup has, in addition to the patch above, the following patches also
> reverted on top of a vanilla 6.4 kernel:
> 
>   4eb3117888a923f6b9b1ad2dd093641c49a63ae5
>   21e26d5e54ab7cfe6b488fd27d4d70956d07e03b
> 
> as 1543b cannot be reverted without those; however, the effect only goes away
> when I also revert 1543b. The kernel has no other patches applied, only these
> three reverts.
> 
> -- end bug report --
> 
> Reported-by: Robert Schwebel <r.schwebel@pengutronix.de>

This tag afaics should be in some or all of the commits -- together with
a Link: or Closes: tag to the report
(https://lore.kernel.org/v9fs/ZK25XZ%2BGpR3KHIB%2F@pengutronix.de/ ).

'Fixes:' tags would seem appropriate here as well. And to get this fixed
in Linux 6.4.y. a "Cc: <stable..." would be great as well.

See Documentation/handling-regressions.rst (
https://docs.kernel.org/process/handling-regressions.html ) and the
links to Documentation/process/submitting-patches.rst or
Documentation/process/5.Posting.rst for details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot ^backmonitor:
https://lore.kernel.org/v9fs/ZK25XZ%2BGpR3KHIB%2F@pengutronix.de/