[1/2] fs/proc: show correct device and inode numbers in /proc/pid/maps

  Device and inode numbers in /proc/pid/maps have to match numbers returned by
statx for the same files.

/proc/pid/maps shows device and inode numbers of vma->vm_file-s. Here is
an issue. If a mapped file is on a stackable file system (e.g.,
overlayfs), vma->vm_file is a backing file whose f_inode is on the
underlying filesystem. To show correct numbers, we need to get a user
file and shows its numbers. The same trick is used to show file paths in
/proc/pid/maps.

But it isn't the end of this story. A file system can manipulate inode numbers
within the getattr callback (e.g., ovl_getattr), so vfs_getattr must be used to
get correct numbers.

Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Signed-off-by: Andrei Vagin <avagin@google.com>
---
 fs/proc/task_mmu.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
  

On Mon, 11 Dec 2023 11:30:47 -0800 Andrei Vagin <avagin@google.com> wrote:

> Device and inode numbers in /proc/pid/maps have to match numbers returned by
> statx for the same files.
> 
> /proc/pid/maps shows device and inode numbers of vma->vm_file-s. Here is
> an issue. If a mapped file is on a stackable file system (e.g.,
> overlayfs), vma->vm_file is a backing file whose f_inode is on the
> underlying filesystem. To show correct numbers, we need to get a user
> file and shows its numbers. The same trick is used to show file paths in
> /proc/pid/maps.
> 
> But it isn't the end of this story. A file system can manipulate inode numbers
> within the getattr callback (e.g., ovl_getattr), so vfs_getattr must be used to
> get correct numbers.

Al, could you please comment on this?

Thanks.

> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -273,9 +273,23 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
>  	const char *name = NULL;
>  
>  	if (file) {
> -		struct inode *inode = file_inode(vma->vm_file);
> -		dev = inode->i_sb->s_dev;
> -		ino = inode->i_ino;
> +		const struct path *path;
> +		struct kstat stat;
> +
> +		path = file_user_path(file);
> +		/*
> +		 * A file system can manipulate inode numbers within the
> +		 * getattr callback (e.g. ovl_getattr).
> +		 */
> +		if (!vfs_getattr_nosec(path, &stat, STATX_INO, AT_STATX_DONT_SYNC)) {
> +			dev = stat.dev;
> +			ino = stat.ino;
> +		} else {
> +			struct inode *inode = d_backing_inode(path->dentry);
> +
> +			dev = inode->i_sb->s_dev;
> +			ino = inode->i_ino;
> +		}
>  		pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
>  	}
>  
> -- 
> 2.43.0.472.g3155946c3a-goog
  

On Mon, 11 Dec 2023 11:30:47 -0800
Andrei Vagin <avagin@google.com> wrote:

> Device and inode numbers in /proc/pid/maps have to match numbers returned by
> statx for the same files.
> 
> /proc/pid/maps shows device and inode numbers of vma->vm_file-s. Here is
> an issue. If a mapped file is on a stackable file system (e.g.,
> overlayfs), vma->vm_file is a backing file whose f_inode is on the
> underlying filesystem. To show correct numbers, we need to get a user
> file and shows its numbers. The same trick is used to show file paths in
> /proc/pid/maps.
> 
> But it isn't the end of this story. A file system can manipulate inode numbers
> within the getattr callback (e.g., ovl_getattr), so vfs_getattr must be used to
> get correct numbers.
> 
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>

We have discussed this with Andrei offlist, so LGTM.

Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

+cc Christian

> Signed-off-by: Andrei Vagin <avagin@google.com>
> ---
>  fs/proc/task_mmu.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 435b61054b5b..abbf96c091ad 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -273,9 +273,23 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
>  	const char *name = NULL;
>  
>  	if (file) {
> -		struct inode *inode = file_inode(vma->vm_file);
> -		dev = inode->i_sb->s_dev;
> -		ino = inode->i_ino;
> +		const struct path *path;
> +		struct kstat stat;
> +
> +		path = file_user_path(file);
> +		/*
> +		 * A file system can manipulate inode numbers within the
> +		 * getattr callback (e.g. ovl_getattr).
> +		 */
> +		if (!vfs_getattr_nosec(path, &stat, STATX_INO, AT_STATX_DONT_SYNC)) {
> +			dev = stat.dev;
> +			ino = stat.ino;
> +		} else {
> +			struct inode *inode = d_backing_inode(path->dentry);
> +
> +			dev = inode->i_sb->s_dev;
> +			ino = inode->i_ino;
> +		}
>  		pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
>  	}
>  
> -- 
> 2.43.0.472.g3155946c3a-goog
>
  

+fsdevel, +overlayfs, +brauner, +miklos

On Mon, Dec 11, 2023 at 9:30 PM Andrei Vagin <avagin@google.com> wrote:
>
> Device and inode numbers in /proc/pid/maps have to match numbers returned by
> statx for the same files.

That statement may be true for regular files.
It is not true for block/char as far as I know.

I think that your fix will break that by displaying the ino/dev
of the block/char reference inode and not their backing rdev inode.

>
> /proc/pid/maps shows device and inode numbers of vma->vm_file-s. Here is
> an issue. If a mapped file is on a stackable file system (e.g.,
> overlayfs), vma->vm_file is a backing file whose f_inode is on the
> underlying filesystem. To show correct numbers, we need to get a user
> file and shows its numbers. The same trick is used to show file paths in
> /proc/pid/maps.

For the *same* trick, see my patch below.

>
> But it isn't the end of this story. A file system can manipulate inode numbers
> within the getattr callback (e.g., ovl_getattr), so vfs_getattr must be used to
> get correct numbers.

This explanation is inaccurate, because it mixes two different overlayfs
traits which are unrelated.
It is true that a filesystem *can* manipulate st_dev in a way that will not
match i_ino and it is true that overlayfs may do that in some non-default
configurations (see [1]), but this is not the reason that you are seeing
mismatches ino/dev in /proc/<pid>/maps.

[1] https://docs.kernel.org/filesystems/overlayfs.html#inode-properties

The reason is that the vma->vm_file is a special internal backing file
which is not otherwise exposed to userspace.
Please see my suggested fix below.

>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> Signed-off-by: Andrei Vagin <avagin@google.com>
> ---
>  fs/proc/task_mmu.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 435b61054b5b..abbf96c091ad 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -273,9 +273,23 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
>         const char *name = NULL;
>
>         if (file) {
> -               struct inode *inode = file_inode(vma->vm_file);
> -               dev = inode->i_sb->s_dev;
> -               ino = inode->i_ino;
> +               const struct path *path;
> +               struct kstat stat;
> +
> +               path = file_user_path(file);
> +               /*
> +                * A file system can manipulate inode numbers within the
> +                * getattr callback (e.g. ovl_getattr).
> +                */
> +               if (!vfs_getattr_nosec(path, &stat, STATX_INO, AT_STATX_DONT_SYNC)) {

Should you prefer to keep this solution it should be constrained to
regular files.

> +                       dev = stat.dev;
> +                       ino = stat.ino;
> +               } else {
> +                       struct inode *inode = d_backing_inode(path->dentry);

d_inode() please.
d_backing_inode()/d_backing_dentry() are relics of an era that never existed
(i.e. union mounts).

> +
> +                       dev = inode->i_sb->s_dev;
> +                       ino = inode->i_ino;
> +               }
>                 pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
>         }
>

Would you mind trying this alternative (untested) patch?
I think it is preferred, because it is simpler.

Thanks,
Amir.

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index ef2eb12906da..5328266be6b5 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -273,7 +273,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
        const char *name = NULL;

        if (file) {
-               struct inode *inode = file_inode(vma->vm_file);
+               struct inode *inode = file_user_inode(vma->vm_file);
+
                dev = inode->i_sb->s_dev;
                ino = inode->i_ino;
                pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 900d0cd55b50..d78412c6fd47 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2581,20 +2581,28 @@ struct file *backing_file_open(const struct
path *user_path, int flags,
 struct path *backing_file_user_path(struct file *f);

 /*
- * file_user_path - get the path to display for memory mapped file
- *
  * When mmapping a file on a stackable filesystem (e.g., overlayfs), the file
  * stored in ->vm_file is a backing file whose f_inode is on the underlying
- * filesystem.  When the mapped file path is displayed to user (e.g. via
- * /proc/<pid>/maps), this helper should be used to get the path to display
- * to the user, which is the path of the fd that user has requested to map.
+ * filesystem.  When the mapped file path and inode number are displayed to
+ * user (e.g. via /proc/<pid>/maps), these helper should be used to get the
+ * path and inode number to display to the user, which is the path of the fd
+ * that user has requested to map and the inode number that would be returned
+ * by fstat() on that same fd.
  */
+/* Get the path to display in /proc/<pid>/maps */
 static inline const struct path *file_user_path(struct file *f)
 {
        if (unlikely(f->f_mode & FMODE_BACKING))
                return backing_file_user_path(f);
        return &f->f_path;
 }
+/* Get the inode whose inode number to display in /proc/<pid>/maps */
+static inline const struct path *file_user_inode(struct file *f)
+{
+       if (unlikely(f->f_mode & FMODE_BACKING))
+               return d_inode(backing_file_user_path(f)->dentry);
+       return file_inode(f);
+}
  

On Tue, Dec 12, 2023 at 07:51:31AM +0200, Amir Goldstein wrote:
> +fsdevel, +overlayfs, +brauner, +miklos
> 
> On Mon, Dec 11, 2023 at 9:30 PM Andrei Vagin <avagin@google.com> wrote:
> >
> > Device and inode numbers in /proc/pid/maps have to match numbers returned by
> > statx for the same files.
> 
> That statement may be true for regular files.
> It is not true for block/char as far as I know.
> 
> I think that your fix will break that by displaying the ino/dev
> of the block/char reference inode and not their backing rdev inode.
> 
> >
> > /proc/pid/maps shows device and inode numbers of vma->vm_file-s. Here is
> > an issue. If a mapped file is on a stackable file system (e.g.,
> > overlayfs), vma->vm_file is a backing file whose f_inode is on the
> > underlying filesystem. To show correct numbers, we need to get a user
> > file and shows its numbers. The same trick is used to show file paths in
> > /proc/pid/maps.
> 
> For the *same* trick, see my patch below.
> 
> >
> > But it isn't the end of this story. A file system can manipulate inode numbers
> > within the getattr callback (e.g., ovl_getattr), so vfs_getattr must be used to
> > get correct numbers.
> 
> This explanation is inaccurate, because it mixes two different overlayfs
> traits which are unrelated.
> It is true that a filesystem *can* manipulate st_dev in a way that will not
> match i_ino and it is true that overlayfs may do that in some non-default
> configurations (see [1]), but this is not the reason that you are seeing
> mismatches ino/dev in /proc/<pid>/maps.
> 
> [1] https://docs.kernel.org/filesystems/overlayfs.html#inode-properties
> 
> The reason is that the vma->vm_file is a special internal backing file
> which is not otherwise exposed to userspace.
> Please see my suggested fix below.
> 
> >
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > Signed-off-by: Andrei Vagin <avagin@google.com>
> > ---
> >  fs/proc/task_mmu.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 435b61054b5b..abbf96c091ad 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -273,9 +273,23 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
> >         const char *name = NULL;
> >
> >         if (file) {
> > -               struct inode *inode = file_inode(vma->vm_file);
> > -               dev = inode->i_sb->s_dev;
> > -               ino = inode->i_ino;
> > +               const struct path *path;
> > +               struct kstat stat;
> > +
> > +               path = file_user_path(file);
> > +               /*
> > +                * A file system can manipulate inode numbers within the
> > +                * getattr callback (e.g. ovl_getattr).
> > +                */
> > +               if (!vfs_getattr_nosec(path, &stat, STATX_INO, AT_STATX_DONT_SYNC)) {
> 
> Should you prefer to keep this solution it should be constrained to
> regular files.

It's also very dicy calling into the filesystem from procfs. You might
hang the system if you end up talking to a hung NFS server or something.
What locks does show_map_vma() hold? And is it safe to call helpers that
might generate io?

> 
> > +                       dev = stat.dev;
> > +                       ino = stat.ino;
> > +               } else {
> > +                       struct inode *inode = d_backing_inode(path->dentry);
> 
> d_inode() please.
> d_backing_inode()/d_backing_dentry() are relics of an era that never existed
> (i.e. union mounts).
> 
> > +
> > +                       dev = inode->i_sb->s_dev;
> > +                       ino = inode->i_ino;
> > +               }
> >                 pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
> >         }
> >
> 
> Would you mind trying this alternative (untested) patch?
> I think it is preferred, because it is simpler.
> 
> Thanks,
> Amir.
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index ef2eb12906da..5328266be6b5 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -273,7 +273,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
>         const char *name = NULL;
> 
>         if (file) {
> -               struct inode *inode = file_inode(vma->vm_file);
> +               struct inode *inode = file_user_inode(vma->vm_file);
> +
>                 dev = inode->i_sb->s_dev;
>                 ino = inode->i_ino;
>                 pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 900d0cd55b50..d78412c6fd47 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2581,20 +2581,28 @@ struct file *backing_file_open(const struct
> path *user_path, int flags,
>  struct path *backing_file_user_path(struct file *f);
> 
>  /*
> - * file_user_path - get the path to display for memory mapped file
> - *
>   * When mmapping a file on a stackable filesystem (e.g., overlayfs), the file
>   * stored in ->vm_file is a backing file whose f_inode is on the underlying
> - * filesystem.  When the mapped file path is displayed to user (e.g. via
> - * /proc/<pid>/maps), this helper should be used to get the path to display
> - * to the user, which is the path of the fd that user has requested to map.
> + * filesystem.  When the mapped file path and inode number are displayed to
> + * user (e.g. via /proc/<pid>/maps), these helper should be used to get the
> + * path and inode number to display to the user, which is the path of the fd
> + * that user has requested to map and the inode number that would be returned
> + * by fstat() on that same fd.
>   */
> +/* Get the path to display in /proc/<pid>/maps */
>  static inline const struct path *file_user_path(struct file *f)
>  {
>         if (unlikely(f->f_mode & FMODE_BACKING))
>                 return backing_file_user_path(f);
>         return &f->f_path;
>  }
> +/* Get the inode whose inode number to display in /proc/<pid>/maps */
> +static inline const struct path *file_user_inode(struct file *f)
> +{
> +       if (unlikely(f->f_mode & FMODE_BACKING))
> +               return d_inode(backing_file_user_path(f)->dentry);
> +       return file_inode(f);
> +}

Way better imho.
  

Hi Amir,

On Mon, Dec 11, 2023 at 9:51 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> +fsdevel, +overlayfs, +brauner, +miklos
>
> On Mon, Dec 11, 2023 at 9:30 PM Andrei Vagin <avagin@google.com> wrote:
> >
> > Device and inode numbers in /proc/pid/maps have to match numbers returned by
> > statx for the same files.
>
> That statement may be true for regular files.
> It is not true for block/char as far as I know.
>
> I think that your fix will break that by displaying the ino/dev
> of the block/char reference inode and not their backing rdev inode.

I think it doesn't break anything here. /proc/pid/maps shows dev of a
filesystem where the device file resides.

7f336b6c3000-7f336b6c4000 rw-p 00000000 00:05 7
  /dev/zero
$ stat /dev/zero
Device: 0,5 Inode: 7           Links: 1     Device type: 1,5

I checked that it works with and without my patch. It doesn't matter, look at
the following comments.

>
> >
> > /proc/pid/maps shows device and inode numbers of vma->vm_file-s. Here is
> > an issue. If a mapped file is on a stackable file system (e.g.,
> > overlayfs), vma->vm_file is a backing file whose f_inode is on the
> > underlying filesystem. To show correct numbers, we need to get a user
> > file and shows its numbers. The same trick is used to show file paths in
> > /proc/pid/maps.
>
> For the *same* trick, see my patch below.

The patch looks good to me. Thanks! Will you send it?

>
> >
> > But it isn't the end of this story. A file system can manipulate inode numbers
> > within the getattr callback (e.g., ovl_getattr), so vfs_getattr must be used to
> > get correct numbers.
>
> This explanation is inaccurate, because it mixes two different overlayfs
> traits which are unrelated.
> It is true that a filesystem *can* manipulate st_dev in a way that will not
> match i_ino and it is true that overlayfs may do that in some non-default
> configurations (see [1]), but this is not the reason that you are seeing
> mismatches ino/dev in /proc/<pid>/maps.
>
> [1] https://docs.kernel.org/filesystems/overlayfs.html#inode-properties
>
> The reason is that the vma->vm_file is a special internal backing file
> which is not otherwise exposed to userspace.
> Please see my suggested fix below.

I understand that this is the main root cause of issues that we have seen.

But when I was preparing this patch, I found that ovl_getattr manipulates
with inode numbers and decided that it can return a different inode number
than file_user_inode(vma->vm_file).i_ino. I am glad that I was wrong and we
don't need to use vfs_getattr here.

>
> >
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > Signed-off-by: Andrei Vagin <avagin@google.com>

<snip>

>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index ef2eb12906da..5328266be6b5 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -273,7 +273,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
>         const char *name = NULL;
>
>         if (file) {
> -               struct inode *inode = file_inode(vma->vm_file);
> +               struct inode *inode = file_user_inode(vma->vm_file);
> +
>                 dev = inode->i_sb->s_dev;
>                 ino = inode->i_ino;
>                 pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 900d0cd55b50..d78412c6fd47 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2581,20 +2581,28 @@ struct file *backing_file_open(const struct
> path *user_path, int flags,
>  struct path *backing_file_user_path(struct file *f);
>
>  /*
> - * file_user_path - get the path to display for memory mapped file
> - *
>   * When mmapping a file on a stackable filesystem (e.g., overlayfs), the file
>   * stored in ->vm_file is a backing file whose f_inode is on the underlying
> - * filesystem.  When the mapped file path is displayed to user (e.g. via
> - * /proc/<pid>/maps), this helper should be used to get the path to display
> - * to the user, which is the path of the fd that user has requested to map.
> + * filesystem.  When the mapped file path and inode number are displayed to
> + * user (e.g. via /proc/<pid>/maps), these helper should be used to get the
> + * path and inode number to display to the user, which is the path of the fd
> + * that user has requested to map and the inode number that would be returned
> + * by fstat() on that same fd.
>   */
> +/* Get the path to display in /proc/<pid>/maps */
>  static inline const struct path *file_user_path(struct file *f)
>  {
>         if (unlikely(f->f_mode & FMODE_BACKING))
>                 return backing_file_user_path(f);
>         return &f->f_path;
>  }
> +/* Get the inode whose inode number to display in /proc/<pid>/maps */
> +static inline const struct path *file_user_inode(struct file *f)

nit: struct inode *

> +{
> +       if (unlikely(f->f_mode & FMODE_BACKING))
> +               return d_inode(backing_file_user_path(f)->dentry);
> +       return file_inode(f);
> +}

Thanks,
Andrei
  

On Tue, Dec 12, 2023 at 1:27 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, Dec 12, 2023 at 07:51:31AM +0200, Amir Goldstein wrote:
> > +fsdevel, +overlayfs, +brauner, +miklos
> >
> > On Mon, Dec 11, 2023 at 9:30 PM Andrei Vagin <avagin@google.com> wrote:
> > >
> > > Device and inode numbers in /proc/pid/maps have to match numbers returned by
> > > statx for the same files.
> >
> > That statement may be true for regular files.
> > It is not true for block/char as far as I know.
> >
> > I think that your fix will break that by displaying the ino/dev
> > of the block/char reference inode and not their backing rdev inode.
> >
> > >
> > > /proc/pid/maps shows device and inode numbers of vma->vm_file-s. Here is
> > > an issue. If a mapped file is on a stackable file system (e.g.,
> > > overlayfs), vma->vm_file is a backing file whose f_inode is on the
> > > underlying filesystem. To show correct numbers, we need to get a user
> > > file and shows its numbers. The same trick is used to show file paths in
> > > /proc/pid/maps.
> >
> > For the *same* trick, see my patch below.
> >
> > >
> > > But it isn't the end of this story. A file system can manipulate inode numbers
> > > within the getattr callback (e.g., ovl_getattr), so vfs_getattr must be used to
> > > get correct numbers.
> >
> > This explanation is inaccurate, because it mixes two different overlayfs
> > traits which are unrelated.
> > It is true that a filesystem *can* manipulate st_dev in a way that will not
> > match i_ino and it is true that overlayfs may do that in some non-default
> > configurations (see [1]), but this is not the reason that you are seeing
> > mismatches ino/dev in /proc/<pid>/maps.
> >
> > [1] https://docs.kernel.org/filesystems/overlayfs.html#inode-properties
> >
> > The reason is that the vma->vm_file is a special internal backing file
> > which is not otherwise exposed to userspace.
> > Please see my suggested fix below.
> >
> > >
> > > Cc: Amir Goldstein <amir73il@gmail.com>
> > > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > > Signed-off-by: Andrei Vagin <avagin@google.com>
> > > ---
> > >  fs/proc/task_mmu.c | 20 +++++++++++++++++---
> > >  1 file changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > index 435b61054b5b..abbf96c091ad 100644
> > > --- a/fs/proc/task_mmu.c
> > > +++ b/fs/proc/task_mmu.c
> > > @@ -273,9 +273,23 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
> > >         const char *name = NULL;
> > >
> > >         if (file) {
> > > -               struct inode *inode = file_inode(vma->vm_file);
> > > -               dev = inode->i_sb->s_dev;
> > > -               ino = inode->i_ino;
> > > +               const struct path *path;
> > > +               struct kstat stat;
> > > +
> > > +               path = file_user_path(file);
> > > +               /*
> > > +                * A file system can manipulate inode numbers within the
> > > +                * getattr callback (e.g. ovl_getattr).
> > > +                */
> > > +               if (!vfs_getattr_nosec(path, &stat, STATX_INO, AT_STATX_DONT_SYNC)) {
> >
> > Should you prefer to keep this solution it should be constrained to
> > regular files.
>
> It's also very dicy calling into the filesystem from procfs. You might
> hang the system if you end up talking to a hung NFS server or something.
> What locks does show_map_vma() hold? And is it safe to call helpers that
> might generate io?

I had the same thoughts when I was thinking about whether it is safe
to use it here
or not. Then I found AT_STATX_DONT_SYNC (don't sync attributes with
the server) and
decided that it should be safe. Anyway, Amir explains that
vfs_getattr_nosec isn't
needed for overlay files.

Thanks,
Andrei
  

On Tue, Dec 12, 2023 at 9:08 PM Andrei Vagin <avagin@google.com> wrote:
>
> Hi Amir,
>
> On Mon, Dec 11, 2023 at 9:51 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > +fsdevel, +overlayfs, +brauner, +miklos
> >
> > On Mon, Dec 11, 2023 at 9:30 PM Andrei Vagin <avagin@google.com> wrote:
> > >
> > > Device and inode numbers in /proc/pid/maps have to match numbers returned by
> > > statx for the same files.
> >
> > That statement may be true for regular files.
> > It is not true for block/char as far as I know.
> >
> > I think that your fix will break that by displaying the ino/dev
> > of the block/char reference inode and not their backing rdev inode.
>
> I think it doesn't break anything here. /proc/pid/maps shows dev of a
> filesystem where the device file resides.
>
> 7f336b6c3000-7f336b6c4000 rw-p 00000000 00:05 7
>   /dev/zero
> $ stat /dev/zero
> Device: 0,5 Inode: 7           Links: 1     Device type: 1,5
>
> I checked that it works with and without my patch. It doesn't matter, look at
> the following comments.
>
> >
> > >
> > > /proc/pid/maps shows device and inode numbers of vma->vm_file-s. Here is
> > > an issue. If a mapped file is on a stackable file system (e.g.,
> > > overlayfs), vma->vm_file is a backing file whose f_inode is on the
> > > underlying filesystem. To show correct numbers, we need to get a user
> > > file and shows its numbers. The same trick is used to show file paths in
> > > /proc/pid/maps.
> >
> > For the *same* trick, see my patch below.
>
> The patch looks good to me. Thanks! Will you send it?
>

I can send it, if you want.
I wouldn't mind if you send it with my Suggested-by though,
as you are already testing it and posting the selftest.

Thanks,
Amir.