Re: [PATCH 1/3] fs, mm: add ->cachestat() file operation

On 2026-06-26 17:18 +0200, Amir Goldstein wrote:
> On Thu, Jun 25, 2026 at 12:36 PM Christian Brauner <[email protected]> wrote:
> >
> > On 2026-06-23 17:34:47+02:00, Amir Goldstein wrote:
> > > On Tue, Jun 23, 2026 at 4:55 PM Pavel Tikhomirov
> > > <[email protected]> wrote:
> > >
> > > > On 6/23/26 15:48, Johannes Weiner wrote:
> > > >
> > > > Yes, AFAIU in overlay when we use realfile we should always use it
> > > > with_ovl_creds(), even though I don't think there is anything cred related
> > > > in filemap_cachestat(), I still think we should follow the common pattern
> > > > other overlay helpers use (similar to ovl_fadvise() and ovl_flush()).
> > > >
> > > > note: Actually some places get ovl_real_file() and use it without
> > > > with_ovl_creds(), e.g.: ovl_read_iter, ovl_write_iter, ovl_splice_read,
> > > > ovl_splice_write. But those look more of an exception than the general
> > > > rule. All other instances use with_ovl_creds().
> > >
> > > Use with_ovl_creds() is a good practice to keep the mental security model,
> > > but it is useless if the security check (can_do_cachestat) is not in the
> > > vfs helper (vfs_cachestat), so please move it there.
> > >
> > > Also it kind of makes more sense to check (flags != 0) in sys_cachestats
> > > before checking permissions.
> > >
> > > > Also there are simingly no other file_operations which return "realfile"
> > > > for further processing, mostly the operation from fsops simply replaces
> > > > general operation with its own logic completely.
> > > >
> > > > Thanks for your review!
> > > >
> > > > ps: Hope overlay maintainers will correctly if I'm getting this wrong.
> > >
> > > I don't think this is wrong per-se, except for can_do_cachestat().
> > >
> > > Just be aware that the real file could change from one cachestat
> > > call to the next (i.e. due to copy up).
> >
> > I'm really grump about adding a new file operation just for a
> > special-sauce system call which is under a CONFIG_* option even. We're
> > not going to set the precedent of piling on custom file operations for a
> > single filesystem everytime someone adds a new system call unless
> > absolutely necessary.
> 
> I had a similar reaction.
> 
> > This looks like it could just use a new helper in
> > fs/backing_file.c that the cachestat thing can call to use the correct
> > file.
> >
> 
> I was considering suggesting this as well.
> Having f_real_file() complement
> 
> but technically, can_do_cachestat() should be checked against
> the overlayfs file/inode AND also against the real file/inode with
> ovl_creds.
> 
> I'd love to be able to provide a backing_file "template" for
> operations, but I don't have a good idea how to do that.
> Do you?

diff --git a/fs/file_table.c b/fs/file_table.c
index c68b8c0a4097..d93059d53e88 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -57,6 +57,8 @@ struct backing_file {
 #ifdef CONFIG_SECURITY
        void *security;
 #endif
+       unsigned int flags;
+       struct cred *backing_cred;
 };

 diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 27cc07738f33..64e07ebc43b0 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -50,6 +50,12 @@ static struct file *ovl_open_realfile(const struct file *file,

                        realfile = backing_file_open(file,
                                                     flags, realpath, current_cred());
+
+                       // Raises ->flags |= BACKING_FILE_CREDS and sets ->creds to ->backing_cred.
+                       backing_file_set_creds(file, ovl_creds(file_inode(file)->i_sb));
+                       // Now add a helper that performs permission checks on
+                       // both creds for that cachestat() thing and returns
+                       // the actual file to operate on.
                }
        }

something like that?