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

[Amir Goldstein]

On Thu, Jun 25, 2026 at 12:36 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On 2026-06-23 17:34:47+02:00, Amir Goldstein wrote:
> > On Tue, Jun 23, 2026 at 4:55 PM Pavel Tikhomirov
> > <ptikhomirov@xxxxxxxxxxxxx> 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?

Thanks,
Amir.