| Message ID | 20231221095410.801061-5-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intruduce stacking filesystem vfs helpers | expand |
On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote: > Assert that the file object is allocated in a backing_file container > so that file_user_path() could be used to display the user path and > not the backing file's path in /proc/<pid>/maps. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/backing-file.c | 27 +++++++++++++++++++++++++++ > fs/overlayfs/file.c | 23 ++++++----------------- > include/linux/backing-file.h | 2 ++ > 3 files changed, 35 insertions(+), 17 deletions(-) > > diff --git a/fs/backing-file.c b/fs/backing-file.c > index 46488de821a2..1ad8c252ec8d 100644 > --- a/fs/backing-file.c > +++ b/fs/backing-file.c > @@ -11,6 +11,7 @@ > #include <linux/fs.h> > #include <linux/backing-file.h> > #include <linux/splice.h> > +#include <linux/mm.h> > > #include "internal.h" > > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, > } > EXPORT_SYMBOL_GPL(backing_file_splice_write); > > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma, > + struct backing_file_ctx *ctx) > +{ > + const struct cred *old_cred; > + int ret; > + > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) || Couldn't that WARN_ON_ONCE() be in every one of these helpers in this series? IOW, when would you ever want to use a backing_file_*() helper on a non-backing file?
On Fri, Dec 22, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote: > > On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote: > > Assert that the file object is allocated in a backing_file container > > so that file_user_path() could be used to display the user path and > > not the backing file's path in /proc/<pid>/maps. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/backing-file.c | 27 +++++++++++++++++++++++++++ > > fs/overlayfs/file.c | 23 ++++++----------------- > > include/linux/backing-file.h | 2 ++ > > 3 files changed, 35 insertions(+), 17 deletions(-) > > > > diff --git a/fs/backing-file.c b/fs/backing-file.c > > index 46488de821a2..1ad8c252ec8d 100644 > > --- a/fs/backing-file.c > > +++ b/fs/backing-file.c > > @@ -11,6 +11,7 @@ > > #include <linux/fs.h> > > #include <linux/backing-file.h> > > #include <linux/splice.h> > > +#include <linux/mm.h> > > > > #include "internal.h" > > > > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, > > } > > EXPORT_SYMBOL_GPL(backing_file_splice_write); > > > > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma, > > + struct backing_file_ctx *ctx) > > +{ > > + const struct cred *old_cred; > > + int ret; > > + > > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) || > > Couldn't that WARN_ON_ONCE() be in every one of these helpers in this > series? IOW, when would you ever want to use a backing_file_*() helper > on a non-backing file? AFAIK, the call chain below backing_file_splice*() and backing_file_*_iter() helpers never end up accessing file_user_path() or assuming that fd of file is installed in fd table, so there is no strong reason to enforce this with an assertion. We can do it for clarity of semantics, in case one of the call chains will start assuming a struct backing_file in the future. WDIT? Thanks, Amir.
On Sat, Dec 23, 2023 at 8:54 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Fri, Dec 22, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote: > > > > On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote: > > > Assert that the file object is allocated in a backing_file container > > > so that file_user_path() could be used to display the user path and > > > not the backing file's path in /proc/<pid>/maps. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/backing-file.c | 27 +++++++++++++++++++++++++++ > > > fs/overlayfs/file.c | 23 ++++++----------------- > > > include/linux/backing-file.h | 2 ++ > > > 3 files changed, 35 insertions(+), 17 deletions(-) > > > > > > diff --git a/fs/backing-file.c b/fs/backing-file.c > > > index 46488de821a2..1ad8c252ec8d 100644 > > > --- a/fs/backing-file.c > > > +++ b/fs/backing-file.c > > > @@ -11,6 +11,7 @@ > > > #include <linux/fs.h> > > > #include <linux/backing-file.h> > > > #include <linux/splice.h> > > > +#include <linux/mm.h> > > > > > > #include "internal.h" > > > > > > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, > > > } > > > EXPORT_SYMBOL_GPL(backing_file_splice_write); > > > > > > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma, > > > + struct backing_file_ctx *ctx) > > > +{ > > > + const struct cred *old_cred; > > > + int ret; > > > + > > > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) || > > > > Couldn't that WARN_ON_ONCE() be in every one of these helpers in this > > series? IOW, when would you ever want to use a backing_file_*() helper > > on a non-backing file? > > AFAIK, the call chain below backing_file_splice*() and backing_file_*_iter() > helpers never end up accessing file_user_path() or assuming that fd of file > is installed in fd table, so there is no strong reason to enforce this with an > assertion. > > We can do it for clarity of semantics, in case one of the call chains will > start assuming a struct backing_file in the future. WDIT? Doh! WDYT? Thanks, Amir.
On Sat, Dec 23, 2023 at 08:56:08AM +0200, Amir Goldstein wrote: > On Sat, Dec 23, 2023 at 8:54 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Fri, Dec 22, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote: > > > > > > On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote: > > > > Assert that the file object is allocated in a backing_file container > > > > so that file_user_path() could be used to display the user path and > > > > not the backing file's path in /proc/<pid>/maps. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > fs/backing-file.c | 27 +++++++++++++++++++++++++++ > > > > fs/overlayfs/file.c | 23 ++++++----------------- > > > > include/linux/backing-file.h | 2 ++ > > > > 3 files changed, 35 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/fs/backing-file.c b/fs/backing-file.c > > > > index 46488de821a2..1ad8c252ec8d 100644 > > > > --- a/fs/backing-file.c > > > > +++ b/fs/backing-file.c > > > > @@ -11,6 +11,7 @@ > > > > #include <linux/fs.h> > > > > #include <linux/backing-file.h> > > > > #include <linux/splice.h> > > > > +#include <linux/mm.h> > > > > > > > > #include "internal.h" > > > > > > > > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, > > > > } > > > > EXPORT_SYMBOL_GPL(backing_file_splice_write); > > > > > > > > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma, > > > > + struct backing_file_ctx *ctx) > > > > +{ > > > > + const struct cred *old_cred; > > > > + int ret; > > > > + > > > > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) || > > > > > > Couldn't that WARN_ON_ONCE() be in every one of these helpers in this > > > series? IOW, when would you ever want to use a backing_file_*() helper > > > on a non-backing file? > > > > AFAIK, the call chain below backing_file_splice*() and backing_file_*_iter() > > helpers never end up accessing file_user_path() or assuming that fd of file > > is installed in fd table, so there is no strong reason to enforce this with an > > assertion. Yeah, but you do use an override_cred() call and you do have that backing_file_ctx. It smells like a bug if anyone would pass in a non-backing file. > > > > We can do it for clarity of semantics, in case one of the call chains will > > start assuming a struct backing_file in the future. WDIT? > > Doh! WDYT? I'd add it as the whole series is predicated on this being used for backing files.
On Sat, Dec 23, 2023 at 3:04 PM Christian Brauner <brauner@kernel.org> wrote: > > On Sat, Dec 23, 2023 at 08:56:08AM +0200, Amir Goldstein wrote: > > On Sat, Dec 23, 2023 at 8:54 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > On Fri, Dec 22, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote: > > > > > Assert that the file object is allocated in a backing_file container > > > > > so that file_user_path() could be used to display the user path and > > > > > not the backing file's path in /proc/<pid>/maps. > > > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > --- > > > > > fs/backing-file.c | 27 +++++++++++++++++++++++++++ > > > > > fs/overlayfs/file.c | 23 ++++++----------------- > > > > > include/linux/backing-file.h | 2 ++ > > > > > 3 files changed, 35 insertions(+), 17 deletions(-) > > > > > > > > > > diff --git a/fs/backing-file.c b/fs/backing-file.c > > > > > index 46488de821a2..1ad8c252ec8d 100644 > > > > > --- a/fs/backing-file.c > > > > > +++ b/fs/backing-file.c > > > > > @@ -11,6 +11,7 @@ > > > > > #include <linux/fs.h> > > > > > #include <linux/backing-file.h> > > > > > #include <linux/splice.h> > > > > > +#include <linux/mm.h> > > > > > > > > > > #include "internal.h" > > > > > > > > > > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, > > > > > } > > > > > EXPORT_SYMBOL_GPL(backing_file_splice_write); > > > > > > > > > > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma, > > > > > + struct backing_file_ctx *ctx) > > > > > +{ > > > > > + const struct cred *old_cred; > > > > > + int ret; > > > > > + > > > > > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) || > > > > > > > > Couldn't that WARN_ON_ONCE() be in every one of these helpers in this > > > > series? IOW, when would you ever want to use a backing_file_*() helper > > > > on a non-backing file? > > > > > > AFAIK, the call chain below backing_file_splice*() and backing_file_*_iter() > > > helpers never end up accessing file_user_path() or assuming that fd of file > > > is installed in fd table, so there is no strong reason to enforce this with an > > > assertion. > > Yeah, but you do use an override_cred() call and you do have that > backing_file_ctx. It smells like a bug if anyone would pass in a > non-backing file. > > > > > > > We can do it for clarity of semantics, in case one of the call chains will > > > start assuming a struct backing_file in the future. WDIT? > > > > Doh! WDYT? > > I'd add it as the whole series is predicated on this being used for > backing files. Sure. Will do. Thanks, Amir.
diff --git a/fs/backing-file.c b/fs/backing-file.c index 46488de821a2..1ad8c252ec8d 100644 --- a/fs/backing-file.c +++ b/fs/backing-file.c @@ -11,6 +11,7 @@ #include <linux/fs.h> #include <linux/backing-file.h> #include <linux/splice.h> +#include <linux/mm.h> #include "internal.h" @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, } EXPORT_SYMBOL_GPL(backing_file_splice_write); +int backing_file_mmap(struct file *file, struct vm_area_struct *vma, + struct backing_file_ctx *ctx) +{ + const struct cred *old_cred; + int ret; + + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) || + WARN_ON_ONCE(ctx->user_file != vma->vm_file)) + return -EIO; + + if (!file->f_op->mmap) + return -ENODEV; + + vma_set_file(vma, file); + + old_cred = override_creds(ctx->cred); + ret = call_mmap(vma->vm_file, vma); + revert_creds(old_cred); + + if (ctx->accessed) + ctx->accessed(ctx->user_file); + + return ret; +} +EXPORT_SYMBOL_GPL(backing_file_mmap); + static int __init backing_aio_init(void) { backing_aio_cachep = kmem_cache_create("backing_aio", diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 69b52d2f9c74..05536964d37f 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -10,7 +10,6 @@ #include <linux/uio.h> #include <linux/uaccess.h> #include <linux/security.h> -#include <linux/mm.h> #include <linux/fs.h> #include <linux/backing-file.h> #include "overlayfs.h" @@ -415,23 +414,13 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) static int ovl_mmap(struct file *file, struct vm_area_struct *vma) { struct file *realfile = file->private_data; - const struct cred *old_cred; - int ret; - - if (!realfile->f_op->mmap) - return -ENODEV; - - if (WARN_ON(file != vma->vm_file)) - return -EIO; - - vma_set_file(vma, realfile); - - old_cred = ovl_override_creds(file_inode(file)->i_sb); - ret = call_mmap(vma->vm_file, vma); - revert_creds(old_cred); - ovl_file_accessed(file); + struct backing_file_ctx ctx = { + .cred = ovl_creds(file_inode(file)->i_sb), + .user_file = file, + .accessed = ovl_file_accessed, + }; - return ret; + return backing_file_mmap(realfile, vma, &ctx); } static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len) diff --git a/include/linux/backing-file.h b/include/linux/backing-file.h index 0546d5b1c9f5..3f1fe1774f1b 100644 --- a/include/linux/backing-file.h +++ b/include/linux/backing-file.h @@ -36,5 +36,7 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, struct file *out, loff_t *ppos, size_t len, unsigned int flags, struct backing_file_ctx *ctx); +int backing_file_mmap(struct file *file, struct vm_area_struct *vma, + struct backing_file_ctx *ctx); #endif /* _LINUX_BACKING_FILE_H */
Assert that the file object is allocated in a backing_file container so that file_user_path() could be used to display the user path and not the backing file's path in /proc/<pid>/maps. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/backing-file.c | 27 +++++++++++++++++++++++++++ fs/overlayfs/file.c | 23 ++++++----------------- include/linux/backing-file.h | 2 ++ 3 files changed, 35 insertions(+), 17 deletions(-)