Re: [PATCH 1/3] backing_file: store user_path_file

On Wed, Mar 18, 2026 at 11:57 AM Christian Brauner <[email protected]> wrote:
>
> On Mon, Mar 16, 2026 at 05:35:56PM -0400, Paul Moore wrote:
> > From: Amir Goldstein <[email protected]>
> >
> > Instead of storing the user_path, store an O_PATH file for the
> > user_path with the original user file creds and a security context.
> >
> > The user_path_file is only exported as a const pointer and its refcnt
> > is initialized to FILE_REF_DEAD, because it is not a refcounted object.
> >
> > The only caller of file_ref_init() is now open coded, so the helper
> > is removed.
> >
> > Signed-off-by: Amir Goldstein <[email protected]>
> > Tested-by: Paul Moore <[email protected]> (SELinux)
> > Acked-by: Gao Xiang <[email protected]> (EROFS)
> > Signed-off-by: Paul Moore <[email protected]>
> > ---
> >  fs/backing-file.c            | 20 ++++++++------
> >  fs/erofs/ishare.c            |  6 ++--
> >  fs/file_table.c              | 53 ++++++++++++++++++++++++++++--------
> >  fs/fuse/passthrough.c        |  3 +-
> >  fs/internal.h                |  5 ++--
> >  fs/overlayfs/dir.c           |  3 +-
> >  fs/overlayfs/file.c          |  1 +
> >  include/linux/backing-file.h | 29 ++++++++++++++++++--
> >  include/linux/file_ref.h     | 10 -------
> >  9 files changed, 90 insertions(+), 40 deletions(-)
> >
> > diff --git a/fs/backing-file.c b/fs/backing-file.c
> > index 45da8600d564..acabeea7efff 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/uio.h>
> >  #include <linux/mm.h>
> >
> >  #include "internal.h"
> > @@ -18,9 +19,10 @@
> >  /**
> >   * backing_file_open - open a backing file for kernel internal use
> >   * @user_path:       path that the user reuqested to open
> > + * @user_cred:       credentials that the user used for open
> >   * @flags:   open flags
> >   * @real_path:       path of the backing file
> > - * @cred:    credentials for open
> > + * @cred:    credentials for open of the backing file
> >   *
> >   * Open a backing file for a stackable filesystem (e.g., overlayfs).
> >   * @user_path may be on the stackable filesystem and @real_path on the
> > @@ -29,19 +31,19 @@
> >   * returned file into a container structure that also stores the stacked
> >   * file's path, which can be retrieved using backing_file_user_path().
> >   */
> > -struct file *backing_file_open(const struct path *user_path, int flags,
> > +struct file *backing_file_open(const struct path *user_path,
> > +                            const struct cred *user_cred, int flags,
> >                              const struct path *real_path,
> >                              const struct cred *cred)
> >  {
> >       struct file *f;
> >       int error;
> >
> > -     f = alloc_empty_backing_file(flags, cred);
> > +     f = alloc_empty_backing_file(flags, cred, user_cred);
> >       if (IS_ERR(f))
> >               return f;
> >
> > -     path_get(user_path);
> > -     backing_file_set_user_path(f, user_path);
> > +     backing_file_open_user_path(f, user_path);
> >       error = vfs_open(real_path, f);
> >       if (error) {
> >               fput(f);
> > @@ -52,7 +54,8 @@ struct file *backing_file_open(const struct path *user_path, int flags,
> >  }
> >  EXPORT_SYMBOL_GPL(backing_file_open);
> >
> > -struct file *backing_tmpfile_open(const struct path *user_path, int flags,
> > +struct file *backing_tmpfile_open(const struct path *user_path,
> > +                               const struct cred *user_cred, int flags,
> >                                 const struct path *real_parentpath,
> >                                 umode_t mode, const struct cred *cred)
> >  {
> > @@ -60,12 +63,11 @@ struct file *backing_tmpfile_open(const struct path *user_path, int flags,
> >       struct file *f;
> >       int error;
> >
> > -     f = alloc_empty_backing_file(flags, cred);
> > +     f = alloc_empty_backing_file(flags, cred, user_cred);
> >       if (IS_ERR(f))
> >               return f;
> >
> > -     path_get(user_path);
> > -     backing_file_set_user_path(f, user_path);
> > +     backing_file_open_user_path(f, user_path);
> >       error = vfs_tmpfile(real_idmap, real_parentpath, f, mode);
> >       if (error) {
> >               fput(f);
> > diff --git a/fs/erofs/ishare.c b/fs/erofs/ishare.c
> > index 829d50d5c717..17a4941d4518 100644
> > --- a/fs/erofs/ishare.c
> > +++ b/fs/erofs/ishare.c
> > @@ -106,15 +106,15 @@ static int erofs_ishare_file_open(struct inode *inode, struct file *file)
> >
> >       if (file->f_flags & O_DIRECT)
> >               return -EINVAL;
> > -     realfile = alloc_empty_backing_file(O_RDONLY|O_NOATIME, current_cred());
> > +     realfile = alloc_empty_backing_file(O_RDONLY|O_NOATIME, current_cred(),
> > +                                         file->f_cred);
> >       if (IS_ERR(realfile))
> >               return PTR_ERR(realfile);
> >       ihold(sharedinode);
> >       realfile->f_op = &erofs_file_fops;
> >       realfile->f_inode = sharedinode;
> >       realfile->f_mapping = sharedinode->i_mapping;
> > -     path_get(&file->f_path);
> > -     backing_file_set_user_path(realfile, &file->f_path);
> > +     backing_file_open_user_path(realfile, &file->f_path);
> >
> >       file_ra_state_init(&realfile->f_ra, file->f_mapping);
> >       realfile->private_data = EROFS_I(inode);
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index aaa5faaace1e..b7dc94656c44 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/task_work.h>
> >  #include <linux/swap.h>
> >  #include <linux/kmemleak.h>
> > +#include <linux/backing-file.h>
> >
> >  #include <linux/atomic.h>
> >
> > @@ -43,11 +44,11 @@ static struct kmem_cache *bfilp_cachep __ro_after_init;
> >
> >  static struct percpu_counter nr_files __cacheline_aligned_in_smp;
> >
> > -/* Container for backing file with optional user path */
> > +/* Container for backing file with optional user path file */
> >  struct backing_file {
> >       struct file file;
> >       union {
> > -             struct path user_path;
> > +             struct file user_path_file;
> >               freeptr_t bf_freeptr;
> >       };
> >  };
> > @@ -56,24 +57,44 @@ struct backing_file {
> >
> >  const struct path *backing_file_user_path(const struct file *f)
> >  {
> > -     return &backing_file(f)->user_path;
> > +     return &backing_file(f)->user_path_file.f_path;
> >  }
> >  EXPORT_SYMBOL_GPL(backing_file_user_path);
> >
> > -void backing_file_set_user_path(struct file *f, const struct path *path)
> > +const struct file *backing_file_user_path_file(const struct file *f)
> >  {
> > -     backing_file(f)->user_path = *path;
> > +     return &backing_file(f)->user_path_file;
> > +}
> > +EXPORT_SYMBOL_GPL(backing_file_user_path_file);
> > +
> > +void backing_file_open_user_path(struct file *f, const struct path *path)
>
> I think this is a bad idea. This should return an error but still
> WARN_ON(). Just make callers handle that error just like we do
> everywhere else.

OK.

>
> > +{
> > +     /* open an O_PATH file to reference the user path - cannot fail */
> > +     WARN_ON(vfs_open(path, &backing_file(f)->user_path_file));
> > +}
> > +EXPORT_SYMBOL_GPL(backing_file_open_user_path);
> > +
> > +static void destroy_file(struct file *f)
> > +{
> > +     security_file_free(f);
> > +     put_cred(f->f_cred);
>
> Note that calling destroy_file() in this way bypasses
> security_file_release(). Presumably this doesn't matter because no LSM
> does a security_alloc_file() for this but it adds a nother wrinkly into
> the cleanup path.
>

This is for Paul to comment on.
The way I see it, security_file_open() was not called on the user path file,
so no reason to call security_file_release()?

It is very much a possibility that LSM would want to allocate security
context for the user path file during backing_file_mmap, when both files
are available in context, so that later mprotect() will have this stored
information in the user path file security context.

But in this case, wouldn't security_file_free() be enough?

>
> >  }
> > -EXPORT_SYMBOL_GPL(backing_file_set_user_path);
> >
> >  static inline void file_free(struct file *f)
> >  {
> > -     security_file_free(f);
> > +     destroy_file(f);
> >       if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
> >               percpu_counter_dec(&nr_files);
> > -     put_cred(f->f_cred);
> >       if (unlikely(f->f_mode & FMODE_BACKING)) {
> > -             path_put(backing_file_user_path(f));
> > +             struct file *user_path_file = &backing_file(f)->user_path_file;
> > +
> > +             /*
> > +              * no refcount on the user_path_file - they die together,
> > +              * so __fput() is not called for user_path_file. path_put()
> > +              * is the only relevant cleanup from __fput().
> > +              */
> > +             destroy_file(user_path_file);
> > +             path_put(&user_path_file->__f_path);
> >               kmem_cache_free(bfilp_cachep, backing_file(f));
> >       } else {
> >               kmem_cache_free(filp_cachep, f);
> > @@ -201,7 +222,7 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
> >        * fget-rcu pattern users need to be able to handle spurious
> >        * refcount bumps we should reinitialize the reused file first.
> >        */
> > -     file_ref_init(&f->f_ref, 1);
> > +     atomic_long_set(&f->f_ref.refcnt, FILE_REF_ONEREF);
>
> No, please don't open-code this. The point is to stop any open-access to
> f_ref. And also below you introduce another atomic_long_set() open-coded
> call as well. Simply adapt file_ref_init() to not do the -1 subtraction
> and use the constants directly.
>

OK.

> >       return 0;
> >  }
> >
> > @@ -290,7 +311,8 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
> >   * This is only for kernel internal use, and the allocate file must not be
> >   * installed into file tables or such.
> >   */
> > -struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
> > +struct file *alloc_empty_backing_file(int flags, const struct cred *cred,
> > +                                   const struct cred *user_cred)
> >  {
> >       struct backing_file *ff;
> >       int error;
> > @@ -305,6 +327,15 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
> >               return ERR_PTR(error);
> >       }
> >
> > +     error = init_file(&ff->user_path_file, O_PATH, user_cred);
> > +     /* user_path_file is not refcounterd - it dies with the backing file */
> > +     atomic_long_set(&ff->user_path_file.f_ref.refcnt, FILE_REF_DEAD);
>
> Please massage this and send that patch. I'll stuff it into a stable vfs
> branch that both Paul and I can merge. Paul can then send his PR.

Sure. I'll try to send it later today.

Thanks,
Amir.