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

[Amir Goldstein]

+fsdevel, +overlayfs, +brauner, +miklos

On Mon, Dec 11, 2023 at 9:30 PM Andrei Vagin <avagin@xxxxxxxxxx> 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@xxxxxxxxx>
> Cc: Alexander Mikhalitsyn <alexander@xxxxxxxxxxxxx>
> Signed-off-by: Andrei Vagin <avagin@xxxxxxxxxx>
> ---
>  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);
+}