Re: [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper

From: Amir Goldstein <amir73il@gmail.com>
To: Christian Brauner <brauner@kernel.org>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	linux-fsdevel@vger.kernel.org,  linux-unionfs@vger.kernel.org
Subject: Re: [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper
Date: Sat, 23 Dec 2023 08:54:27 +0200	[thread overview]
Message-ID: <CAOQ4uxiL=DckFZqq1APPUaWwWynH6mAJk+VcKO46dwGD521FYw@mail.gmail.com> (raw)
In-Reply-To: <20231222-gespeichert-prall-3183a634baae@brauner>

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.