+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); +}
