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

On Tue, Jun 30, 2026 at 9:55=E2=80=AFAM Christian Brauner <[email protected]=
rg> wrote:
>
> On 2026-06-26 17:18 +0200, Amir Goldstein wrote:
> > On Thu, Jun 25, 2026 at 12:36=E2=80=AFPM Christian Brauner <brauner@ker=
nel.org> wrote:
> > >
> > > On 2026-06-23 17:34:47+02:00, Amir Goldstein wrote:
> > > > On Tue, Jun 23, 2026 at 4:55=E2=80=AFPM 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 i=
t
> > > > > with_ovl_creds(), even though I don't think there is anything cre=
d 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 ge=
neral
> > > > > 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 i=
n the
> > > > vfs helper (vfs_cachestat), so please move it there.
> > > >
> > > > Also it kind of makes more sense to check (flags !=3D 0) in sys_cac=
hestats
> > > > before checking permissions.
> > > >
> > > > > Also there are simingly no other file_operations which return "re=
alfile"
> > > > > for further processing, mostly the operation from fsops simply re=
places
> > > > > general operation with its own logic completely.
> > > > >
> > > > > Thanks for your review!
> > > > >
> > > > > ps: Hope overlay maintainers will correctly if I'm getting this w=
rong.
> > > >
> > > > 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'r=
e
> > > not going to set the precedent of piling on custom file operations fo=
r 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 correc=
t
> > > 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 fil=
e *file,
>
>                         realfile =3D backing_file_open(file,
>                                                      flags, realpath, cur=
rent_cred());
> +
> +                       // Raises ->flags |=3D BACKING_FILE_CREDS and set=
s ->creds to ->backing_cred.
> +                       backing_file_set_creds(file, ovl_creds(file_inode=
(file)->i_sb));
> +                       // Now add a helper that performs permission chec=
ks on
> +                       // both creds for that cachestat() thing and retu=
rns
> +                       // the actual file to operate on.
>                 }
>         }
>
> something like that?
>

Confused.
It seems strange to have backing_cred in this scope.
I would expect having user_cred same as user_path.
backing_file gets you from the real file to properties of the "user"
facing file.

The cachestat() syscall has a reference to the user facing file not to
the real file.
The helper would need something like f_real_file() to get to real file
from user facing file.

Anyway I am not seeing the big picture clearly how this could turn to
be nice code.
Maybe lack of imagination on my part.

Thanks,
Amir.