Hi Amir, On Mon, Dec 11, 2023 at 9:51 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > +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. 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@xxxxxxxxx> > > Cc: Alexander Mikhalitsyn <alexander@xxxxxxxxxxxxx> > > Signed-off-by: Andrei Vagin <avagin@xxxxxxxxxx> <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
