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
