* 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ messages in thread
* [PATCH 1/2 v2] fs/proc: show correct device and inode numbers in /proc/pid/maps @ 2023-12-14 6:44 Andrei Vagin 2023-12-14 6:44 ` [PATCH 2/2] selftests/overlayfs: verify " Andrei Vagin 0 siblings, 1 reply; 8+ messages in thread From: Andrei Vagin @ 2023-12-14 6:44 UTC (permalink / raw) To: Andrew Morton, Amir Goldstein Cc: linux-kernel, Alexander Mikhalitsyn, Christian Brauner, Alexander Viro, linux-fsdevel, overlayfs, Miklos Szeredi, Andrei Vagin /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. Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com> Suggested-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: Andrei Vagin <avagin@google.com> --- v2: Amir explained that vfs_getattr isn't needed, because file_user_inode(vma->vm_file).i_ino always matches an inode number returned by statx. fs/proc/task_mmu.c | 3 ++- include/linux/fs.h | 18 +++++++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 435b61054b5b..1801e409a061 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); + const 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 98b7a7a8c42e..838ccfc63323 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2523,20 +2523,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 helpers 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 inode *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); +} static inline struct file *file_clone_open(struct file *file) { -- 2.43.0.472.g3155946c3a-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] selftests/overlayfs: verify device and inode numbers in /proc/pid/maps 2023-12-14 6:44 [PATCH 1/2 v2] " Andrei Vagin @ 2023-12-14 6:44 ` Andrei Vagin 2023-12-14 8:03 ` Amir Goldstein 0 siblings, 1 reply; 8+ messages in thread From: Andrei Vagin @ 2023-12-14 6:44 UTC (permalink / raw) To: Andrew Morton, Amir Goldstein Cc: linux-kernel, Alexander Mikhalitsyn, Christian Brauner, Alexander Viro, linux-fsdevel, overlayfs, Miklos Szeredi, Andrei Vagin 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> --- 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..e19ab0e85709 --- /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 %llu", + &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 related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] selftests/overlayfs: verify device and inode numbers in /proc/pid/maps 2023-12-14 6:44 ` [PATCH 2/2] selftests/overlayfs: verify " Andrei Vagin @ 2023-12-14 8:03 ` Amir Goldstein 0 siblings, 0 replies; 8+ messages in thread From: Amir Goldstein @ 2023-12-14 8:03 UTC (permalink / raw) To: Andrei Vagin Cc: Andrew Morton, linux-kernel, Alexander Mikhalitsyn, Christian Brauner, Alexander Viro, linux-fsdevel, overlayfs, Miklos Szeredi On Thu, Dec 14, 2023 at 8:44 AM 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> 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..e19ab0e85709 > --- /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 %llu", > + &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] 8+ messages in thread
end of thread, other threads:[~2023-12-14 8:03 UTC | newest]
Thread overview: 8+ 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
2023-12-14 6:44 [PATCH 1/2 v2] " Andrei Vagin
2023-12-14 6:44 ` [PATCH 2/2] selftests/overlayfs: verify " Andrei Vagin
2023-12-14 8:03 ` 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).