On 6/30/26 09:55, Christian Brauner 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.
I'm not sure I fully understand it, sorry.
The idea seems to be to replace file operation customization helper
->cachestat() with some way for cachestat syscall to access a backing file
directly. At the same time providing the credentials from overlay with which to
check permissions on this backing file.
I think the inherent recursiveness of overlay (e.g. one can mount overlayfs
where lower fs is again an overlay) is effectively making us check permissions
recursively for each level of backing file, so it would be hard to have just
one backing file. But maybe I miss something.
> }
> }
>
> something like that?
>
--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
