On Tue, Jun 30, 2026 at 9:55 AM Christian Brauner <[email protected]> wrote: > > 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? >
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.
