Re: [PATCH] erofs: use the opener's credential when verifing metadata accesses

Hi Christian,

On 2026/5/11 21:51, Christian Brauner wrote:
> On Fri, May 08, 2026 at 04:39:15PM +0800, Gao Xiang wrote:
>> Hi Christiph,
>>
>> On 2026/5/8 16:24, Tatsuyuki Ishi wrote:
>>> On Fri, May 8, 2026 at 5:20 PM Christoph Hellwig <[email protected]> wrote:
>>>
>>>> On Tue, May 05, 2026 at 11:56:15PM +0800, Gao Xiang wrote:
>>>>> Similar to commit 905eeb2b7c33 ("erofs: impersonate the opener's
>>>>> credentials when accessing backing file"), rw_verify_area() needs
>>>>> the same too.
>>>>
>>>> Two things here:
>>
>> Let me use Tatsuyuki's reply to address your two comments.
>>
>>>>
>>>>    - rw_verify_area is a helper for use inside the VFS and file system
>>>>      read/write method implementation.  Erofs as a user of the VFS should
>>>>      not use it at all.
>>
>> Currently EROFS file-backed mount metadata is directly using underlay
>> fs page cache, which is mainly used for composefs, etc. to avoid
>> different EROFS instances have their own EROFS page cache for the
>> same underlay backing file and avoid unnecessary copies into them.
>> --- That is also what composefs once did in their codebase.
>>
>> Since EROFS just read the underlayfs page cache and does _not_
>> touch anything inside the underlay page cache itself, so I guess
>> it's fine?
>>
>> On the other hand, we talked a bit commit f2fed441c69b ("loop:
>> stop using vfs_iter_{read,write} for buffered I/O") in another
>> private thread related to fanotify, which lacks proper
>> rw_verify_area() as well, since it called into raw read/write
>> iter methods instead of using the previous vfs_iter_{read,write}.
>>
>>>>    - using the opener credentials when accessing the backing file seems
>>>>      wrong.  The entity accessing it is the file system, so it should
>>>>      have system or mounter credentials, not that of someone causing
>>>>      metadata / fs data access.  And this applies to all access by
>>>>      a file system backed by a backing file.
>>>>
>>>
>>> I think there's probably some confusion of terminology here. buf->file is
>>> opened with the mounter's credentials, so we are impersonating the mounter
>>> here. Perhaps the commit message could describe that more clearly. Same for
>>> the previous patches mentioned.
>>
>> Here "opener" means the mounter as Tatsuyuki mentioned, I just
>> follows Tatsuyuki's term, but it just means mounter credentials
>> indeed.
> 
> We're slowly reinventing overlayfs I see. ;) I think it's probably fine
> but it's also rather sketchy to mess around with permissions like that.
> Mainly because I don't think we have any actual page cache permission
> model. It's inherently shared beetween everyone and this kinda tries to
> bolt permissions on top to not make it so. Probably fine here but also a
> bit wonky.

Loop devices just purely use kernel cred instead, I think using
the mounter cred is more reasonable and safer than the kernel
cred.

Anyway, I think this cred part is less controversy.. The main
issue out of Christoph is still the metadata path: I tend to use
the underlay inode page cache for temporary RO access since it's
efficient and cache-friendly; and for immutable models we
shouldn't care too much about the invalidation, etc. since there
is no need to rely on the locking to keep the underlay data in
a strict way.


Thanks,
Gao Xiang