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

On Wed, Mar 18, 2026 at 8:07 AM Amir Goldstein <[email protected]> wrote:
> 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/file_table.c b/fs/file_table.c
> > > index aaa5faaace1e..b7dc94656c44 100644
> > > --- a/fs/file_table.c
> > > +++ b/fs/file_table.c
> > > @@ -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.

That's good, I can remove the FMODE_OPENED sanity check in
selinux_file_mprotect() now.

> > > +{
> > > +     /* 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.

The alloc_empty_backing_file() function will call init_file() on the
O_PATH file which in turnwill call security_file_alloc().  We depend
on that behavior as we need to set the creds properly on the file.

The tricky bit is that security_file_release() is currently only used
by IMA/EVM; it was created when we folded IMA/EVM into the LSM
framework, making them proper LSMs and cleaning up a lot of the hook
calls in the VFS.  The commit description provides some info on the
hook:

   IMA calculates at file close the new digest of the file content
   and writes it to security.ima, so that appraisal at next file
   access succeeds.

   The new hook cannot return an error and cannot cause the operation
   to be reverted.

As O_PATH files never go through the
security_file_open()/security_file_post_open() path, O_PATH files
should be ignored by IMA/EVM (which makes sense, as there is no data
or xattrs to measure, update, etc.).  For that reason, skipping
security_file_release() should be okay in this particular case.

Other LSMs which allocate file specific state, e.g. AppArmor, in
security_file_alloc() should free that state in security_file_free()
which is handled in Amir's patch.

> 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?

See my comments above.  If we wanted to look into removing the
destroy_file() special case for the user_path file hanging off the
backing file I'm happy to work with you guys in the future to sort
that out, but I'd prefer to tackle that at a later date as it could
potentially have a much larger footprint than this patchset.

-- 
paul-moore.com