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? Thanks, Amir.
