* Re: [PATCH 2/2] selftests/overlayfs: verify device and inode numbers in /proc/pid/maps
[not found] ` <20231211193048.580691-2-avagin@google.com>
@ 2023-12-12 5:17 ` Amir Goldstein
0 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2023-12-12 5:17 UTC (permalink / raw)
To: Andrei Vagin
Cc: Andrew Morton, linux-kernel, Alexander Mikhalitsyn, linux-fsdevel,
Christian Brauner
+fsdevel, +brauner
On Mon, Dec 11, 2023 at 9:30 PM Andrei Vagin <avagin@google.com> wrote:
>
> When mapping a file on overlayfs, the file stored in ->vm_file is a
> backing file whose f_inode is on the underlying filesystem. We need to
> verify that /proc/pid/maps contains numbers of the overlayfs file, but
> not its backing file.
>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> Signed-off-by: Andrei Vagin <avagin@google.com>
Thanks for the fix and test.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> tools/testing/selftests/Makefile | 1 +
> .../filesystems/overlayfs/.gitignore | 2 +
> .../selftests/filesystems/overlayfs/Makefile | 7 +
> .../filesystems/overlayfs/dev_in_maps.c | 182 ++++++++++++++++++
> .../selftests/filesystems/overlayfs/log.h | 26 +++
> 5 files changed, 218 insertions(+)
> create mode 100644 tools/testing/selftests/filesystems/overlayfs/.gitignore
> create mode 100644 tools/testing/selftests/filesystems/overlayfs/Makefile
> create mode 100644 tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
> create mode 100644 tools/testing/selftests/filesystems/overlayfs/log.h
>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 3b2061d1c1a5..0939a40abb28 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -26,6 +26,7 @@ TARGETS += filesystems
> TARGETS += filesystems/binderfs
> TARGETS += filesystems/epoll
> TARGETS += filesystems/fat
> +TARGETS += filesystems/overlayfs
> TARGETS += firmware
> TARGETS += fpu
> TARGETS += ftrace
> diff --git a/tools/testing/selftests/filesystems/overlayfs/.gitignore b/tools/testing/selftests/filesystems/overlayfs/.gitignore
> new file mode 100644
> index 000000000000..52ae618fdd98
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/overlayfs/.gitignore
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +dev_in_maps
> diff --git a/tools/testing/selftests/filesystems/overlayfs/Makefile b/tools/testing/selftests/filesystems/overlayfs/Makefile
> new file mode 100644
> index 000000000000..56b2b48a765b
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/overlayfs/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +TEST_GEN_PROGS := dev_in_maps
> +
> +CFLAGS := -Wall -Werror
> +
> +include ../../lib.mk
> diff --git a/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c b/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
> new file mode 100644
> index 000000000000..08497c2e10a3
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +
> +#include <inttypes.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +
> +#include <linux/unistd.h>
> +#include <linux/types.h>
> +#include <linux/mount.h>
> +#include <sys/syscall.h>
> +#include <sys/stat.h>
> +#include <sys/mount.h>
> +#include <sys/mman.h>
> +#include <sched.h>
> +#include <fcntl.h>
> +
> +#include "../../kselftest.h"
> +#include "log.h"
> +
> +static int sys_fsopen(const char *fsname, unsigned int flags)
> +{
> + return syscall(__NR_fsopen, fsname, flags);
> +}
> +
> +static int sys_fsconfig(int fd, unsigned int cmd, const char *key, const char *value, int aux)
> +{
> + return syscall(__NR_fsconfig, fd, cmd, key, value, aux);
> +}
> +
> +static int sys_fsmount(int fd, unsigned int flags, unsigned int attr_flags)
> +{
> + return syscall(__NR_fsmount, fd, flags, attr_flags);
> +}
> +
> +static int sys_move_mount(int from_dfd, const char *from_pathname,
> + int to_dfd, const char *to_pathname,
> + unsigned int flags)
> +{
> + return syscall(__NR_move_mount, from_dfd, from_pathname, to_dfd, to_pathname, flags);
> +}
> +
> +static long get_file_dev_and_inode(void *addr, struct statx *stx)
> +{
> + char buf[4096];
> + FILE *mapf;
> +
> + mapf = fopen("/proc/self/maps", "r");
> + if (mapf == NULL)
> + return pr_perror("fopen(/proc/self/maps)");
> +
> + while (fgets(buf, sizeof(buf), mapf)) {
> + unsigned long start, end;
> + uint32_t maj, min;
> + __u64 ino;
> +
> + if (sscanf(buf, "%lx-%lx %*s %*s %x:%x %llx",
> + &start, &end, &maj, &min, &ino) != 5)
> + return pr_perror("unable to parse: %s", buf);
> + if (start == (unsigned long)addr) {
> + stx->stx_dev_major = maj;
> + stx->stx_dev_minor = min;
> + stx->stx_ino = ino;
> + return 0;
> + }
> + }
> +
> + return pr_err("unable to find the mapping");
> +}
> +
> +static int ovl_mount(void)
> +{
> + int tmpfs, fsfd, ovl;
> +
> + fsfd = sys_fsopen("tmpfs", 0);
> + if (fsfd == -1)
> + return pr_perror("fsopen(tmpfs)");
> +
> + if (sys_fsconfig(fsfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0) == -1)
> + return pr_perror("FSCONFIG_CMD_CREATE");
> +
> + tmpfs = sys_fsmount(fsfd, 0, 0);
> + if (tmpfs == -1)
> + return pr_perror("fsmount");
> +
> + close(fsfd);
> +
> + /* overlayfs can't be constructed on top of a detached mount. */
> + if (sys_move_mount(tmpfs, "", AT_FDCWD, "/tmp", MOVE_MOUNT_F_EMPTY_PATH))
> + return pr_perror("move_mount");
> + close(tmpfs);
> +
> + if (mkdir("/tmp/w", 0755) == -1 ||
> + mkdir("/tmp/u", 0755) == -1 ||
> + mkdir("/tmp/l", 0755) == -1)
> + return pr_perror("mkdir");
> +
> + fsfd = sys_fsopen("overlay", 0);
> + if (fsfd == -1)
> + return pr_perror("fsopen(overlay)");
> + if (sys_fsconfig(fsfd, FSCONFIG_SET_STRING, "source", "test", 0) == -1 ||
> + sys_fsconfig(fsfd, FSCONFIG_SET_STRING, "lowerdir", "/tmp/l", 0) == -1 ||
> + sys_fsconfig(fsfd, FSCONFIG_SET_STRING, "upperdir", "/tmp/u", 0) == -1 ||
> + sys_fsconfig(fsfd, FSCONFIG_SET_STRING, "workdir", "/tmp/w", 0) == -1)
> + return pr_perror("fsconfig");
> + if (sys_fsconfig(fsfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0) == -1)
> + return pr_perror("fsconfig");
> + ovl = sys_fsmount(fsfd, 0, 0);
> + if (ovl == -1)
> + return pr_perror("fsmount");
> +
> + return ovl;
> +}
> +
> +/*
> + * Check that the file device and inode shown in /proc/pid/maps match values
> + * returned by stat(2).
> + */
> +static int test(void)
> +{
> + struct statx stx, mstx;
> + int ovl, fd;
> + void *addr;
> +
> + ovl = ovl_mount();
> + if (ovl == -1)
> + return -1;
> +
> + fd = openat(ovl, "test", O_RDWR | O_CREAT, 0644);
> + if (fd == -1)
> + return pr_perror("openat");
> +
> + addr = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0);
> + if (addr == MAP_FAILED)
> + return pr_perror("mmap");
> +
> + if (get_file_dev_and_inode(addr, &mstx))
> + return -1;
> + if (statx(fd, "", AT_EMPTY_PATH | AT_STATX_SYNC_AS_STAT, STATX_INO, &stx))
> + return pr_perror("statx");
> +
> + if (stx.stx_dev_major != mstx.stx_dev_major ||
> + stx.stx_dev_minor != mstx.stx_dev_minor ||
> + stx.stx_ino != mstx.stx_ino)
> + return pr_fail("unmatched dev:ino %x:%x:%llx (expected %x:%x:%llx)\n",
> + mstx.stx_dev_major, mstx.stx_dev_minor, mstx.stx_ino,
> + stx.stx_dev_major, stx.stx_dev_minor, stx.stx_ino);
> +
> + ksft_test_result_pass("devices are matched\n");
> + return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int fsfd;
> +
> + fsfd = sys_fsopen("overlay", 0);
> + if (fsfd == -1) {
> + ksft_test_result_skip("unable to create overlay mount\n");
> + return 1;
> + }
> + close(fsfd);
> +
> + /* Create a new mount namespace to not care about cleaning test mounts. */
> + if (unshare(CLONE_NEWNS) == -1) {
> + ksft_test_result_skip("unable to create a new mount namespace\n");
> + return 1;
> + }
> +
> + if (mount(NULL, "/", NULL, MS_SLAVE | MS_REC, NULL) == -1) {
> + pr_perror("mount");
> + return 1;
> + }
> +
> + ksft_set_plan(1);
> +
> + if (test())
> + return 1;
> +
> + ksft_exit_pass();
> + return 0;
> +}
> diff --git a/tools/testing/selftests/filesystems/overlayfs/log.h b/tools/testing/selftests/filesystems/overlayfs/log.h
> new file mode 100644
> index 000000000000..db64df2a8483
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/overlayfs/log.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __SELFTEST_TIMENS_LOG_H__
> +#define __SELFTEST_TIMENS_LOG_H__
> +
> +#define pr_msg(fmt, lvl, ...) \
> + ksft_print_msg("[%s] (%s:%d)\t" fmt "\n", \
> + lvl, __FILE__, __LINE__, ##__VA_ARGS__)
> +
> +#define pr_p(func, fmt, ...) func(fmt ": %m", ##__VA_ARGS__)
> +
> +#define pr_err(fmt, ...) \
> + ({ \
> + ksft_test_result_error(fmt "\n", ##__VA_ARGS__); \
> + -1; \
> + })
> +
> +#define pr_fail(fmt, ...) \
> + ({ \
> + ksft_test_result_fail(fmt, ##__VA_ARGS__); \
> + -1; \
> + })
> +
> +#define pr_perror(fmt, ...) pr_p(pr_err, fmt, ##__VA_ARGS__)
> +
> +#endif
> --
> 2.43.0.472.g3155946c3a-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] fs/proc: show correct device and inode numbers in /proc/pid/maps
[not found] <20231211193048.580691-1-avagin@google.com>
[not found] ` <20231211193048.580691-2-avagin@google.com>
@ 2023-12-12 5:51 ` Amir Goldstein
2023-12-12 9:27 ` Christian Brauner
2023-12-12 19:08 ` Andrei Vagin
1 sibling, 2 replies; 6+ messages in thread
From: Amir Goldstein @ 2023-12-12 5:51 UTC (permalink / raw)
To: Andrei Vagin
Cc: Andrew Morton, linux-kernel, Alexander Mikhalitsyn,
Christian Brauner, linux-fsdevel, Miklos Szeredi, overlayfs
+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);
+}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] fs/proc: show correct device and inode numbers in /proc/pid/maps
2023-12-12 5:51 ` [PATCH 1/2] fs/proc: show correct " Amir Goldstein
@ 2023-12-12 9:27 ` Christian Brauner
2023-12-12 19:19 ` Andrei Vagin
2023-12-12 19:08 ` Andrei Vagin
1 sibling, 1 reply; 6+ messages in thread
From: Christian Brauner @ 2023-12-12 9:27 UTC (permalink / raw)
To: Amir Goldstein
Cc: Andrei Vagin, Andrew Morton, linux-kernel, Alexander Mikhalitsyn,
linux-fsdevel, Miklos Szeredi, overlayfs
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] fs/proc: show correct device and inode numbers in /proc/pid/maps
2023-12-12 5:51 ` [PATCH 1/2] fs/proc: show correct " Amir Goldstein
2023-12-12 9:27 ` Christian Brauner
@ 2023-12-12 19:08 ` Andrei Vagin
2023-12-13 17:05 ` Amir Goldstein
1 sibling, 1 reply; 6+ messages in thread
From: Andrei Vagin @ 2023-12-12 19:08 UTC (permalink / raw)
To: Amir Goldstein
Cc: Andrew Morton, linux-kernel, Alexander Mikhalitsyn,
Christian Brauner, linux-fsdevel, Miklos Szeredi, overlayfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] fs/proc: show correct device and inode numbers in /proc/pid/maps
2023-12-12 9:27 ` Christian Brauner
@ 2023-12-12 19:19 ` Andrei Vagin
0 siblings, 0 replies; 6+ messages in thread
From: Andrei Vagin @ 2023-12-12 19:19 UTC (permalink / raw)
To: Christian Brauner
Cc: Amir Goldstein, Andrew Morton, linux-kernel,
Alexander Mikhalitsyn, linux-fsdevel, Miklos Szeredi, overlayfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] fs/proc: show correct device and inode numbers in /proc/pid/maps
2023-12-12 19:08 ` Andrei Vagin
@ 2023-12-13 17:05 ` Amir Goldstein
0 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2023-12-13 17:05 UTC (permalink / raw)
To: Andrei Vagin
Cc: Andrew Morton, linux-kernel, Alexander Mikhalitsyn,
Christian Brauner, linux-fsdevel, Miklos Szeredi, overlayfs
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-13 17:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231211193048.580691-1-avagin@google.com>
[not found] ` <20231211193048.580691-2-avagin@google.com>
2023-12-12 5:17 ` [PATCH 2/2] selftests/overlayfs: verify device and inode numbers in /proc/pid/maps Amir Goldstein
2023-12-12 5:51 ` [PATCH 1/2] fs/proc: show correct " Amir Goldstein
2023-12-12 9:27 ` Christian Brauner
2023-12-12 19:19 ` Andrei Vagin
2023-12-12 19:08 ` Andrei Vagin
2023-12-13 17:05 ` Amir Goldstein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).