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

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

[Gao Xiang]

Similar to commit 905eeb2b7c33 ("erofs: impersonate the opener's
credentials when accessing backing file"), rw_verify_area() needs
the same too.

Fixes: 307210c262a2 ("erofs: verify metadata accesses for file-backed mounts")
Cc: Carlos Llamas <cmllamas@google.com>
Cc: Sandeep Dhavale <dhavale@google.com>
Cc: Tatsuyuki Ishi <ishitatsuyuki@google.com>
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
Can we verify this patch resolve the android-mainline issue?

 fs/erofs/data.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index b2c12c5856ac..51b8e860b6b2 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -40,9 +40,11 @@ void *erofs_bread(struct erofs_buf *buf, erofs_off_t offset, bool need_kmap)
 	 */
 	if (buf->file) {
 		fpos = (loff_t)index << PAGE_SHIFT;
-		err = rw_verify_area(READ, buf->file, &fpos, PAGE_SIZE);
-		if (err < 0)
-			return ERR_PTR(err);
+		scoped_with_creds(buf->file->f_cred) {
+			err = rw_verify_area(READ, buf->file, &fpos, PAGE_SIZE);
+			if (err < 0)
+				return ERR_PTR(err);
+		}
 	}
 
 	if (buf->page) {
-- 
2.43.5

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

[Carlos Llamas]

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.
> 
> Fixes: 307210c262a2 ("erofs: verify metadata accesses for file-backed mounts")
> Cc: Carlos Llamas <cmllamas@google.com>
> Cc: Sandeep Dhavale <dhavale@google.com>
> Cc: Tatsuyuki Ishi <ishitatsuyuki@google.com>
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> ---
> Can we verify this patch resolve the android-mainline issue?

Yes, it does. Thanks!

Tested-by: Carlos Llamas <cmllamas@google.com>

[PATCH v2] erofs: use the opener's credential when verifying metadata accesses

[Gao Xiang]

Similar to commit 905eeb2b7c33 ("erofs: impersonate the opener's
credentials when accessing backing file"), rw_verify_area() needs
the same too.

Fixes: 307210c262a2 ("erofs: verify metadata accesses for file-backed mounts")
Cc: Carlos Llamas <cmllamas@google.com>
Cc: Sandeep Dhavale <dhavale@google.com>
Cc: Tatsuyuki Ishi <ishitatsuyuki@google.com>
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
v2:
 - apply sashiko's suggestion
   https://sashiko.dev/#/patchset/20260505155615.2719500-1-hsiangkao%40linux.alibaba.com

 fs/erofs/data.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index 44da21c9d777..d6f6035fd714 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -29,6 +29,7 @@ void erofs_put_metabuf(struct erofs_buf *buf)
 void *erofs_bread(struct erofs_buf *buf, erofs_off_t offset, bool need_kmap)
 {
 	pgoff_t index = (buf->off + offset) >> PAGE_SHIFT;
+	const struct cred *old_cred = NULL;
 	struct folio *folio = NULL;
 	loff_t fpos;
 	int err;
@@ -40,9 +41,12 @@ void *erofs_bread(struct erofs_buf *buf, erofs_off_t offset, bool need_kmap)
 	 */
 	if (buf->file) {
 		fpos = (loff_t)index << PAGE_SHIFT;
+		old_cred = override_creds(buf->file->f_cred);
 		err = rw_verify_area(READ, buf->file, &fpos, PAGE_SIZE);
-		if (err < 0)
+		if (err < 0) {
+			revert_creds(old_cred);
 			return ERR_PTR(err);
+		}
 	}
 
 	if (buf->page) {
@@ -53,6 +57,8 @@ void *erofs_bread(struct erofs_buf *buf, erofs_off_t offset, bool need_kmap)
 	if (!folio || !folio_contains(folio, index)) {
 		erofs_put_metabuf(buf);
 		folio = read_mapping_folio(buf->mapping, index, buf->file);
+		if (old_cred)
+			revert_creds(old_cred);
 		if (IS_ERR(folio))
 			return folio;
 	}
-- 
2.43.5

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

[Christoph Hellwig]

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:

 - 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.
 - 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.

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

[Tatsuyuki Ishi]

>  - 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.

[resend: previous mail was rejected due to HTML]

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

[Gao Xiang]

Hi Christiph,

On 2026/5/8 16:24, Tatsuyuki Ishi wrote:
> On Fri, May 8, 2026 at 5:20 PM Christoph Hellwig <hch@infradead.org> 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.

Thanks,
Gao Xiang

> 

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

[Christoph Hellwig]

On Fri, May 08, 2026 at 04:39:15PM +0800, Gao Xiang wrote:
> 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?

At the micro-level this does mean erofs needs to do the checks itself.
OTOH it means this whole scheme is completely broken.  The page cache
is owned by the file system, so erofs can't simply poke into it.

Now for reads it mostly works on the most common disk-based file systems,
but it does create lots of problem for slightly more complex ones like
network/clustered or synthetic file systems.  It also really breaks
layering, so we need to fix it.  Not sure what would be best, but I'd be
tempted to have a cross-instance cache maintained by erofs and filled
using in-kernel direct I/O.  IFF the page policies work great for you
that even could be a synthetic inode/mapping.

> 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}.

Note that this does not add the bypass, just extends it to both I/O
types.  But yes, this breaks fanotify.  We actually have quite a few
raw ->read_iter/->write_iter calls, so this might need more structured
treatment.

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

[Gao Xiang]

On 2026/5/8 16:51, Christoph Hellwig wrote:
> On Fri, May 08, 2026 at 04:39:15PM +0800, Gao Xiang wrote:
>> 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?
> 
> At the micro-level this does mean erofs needs to do the checks itself.
> OTOH it means this whole scheme is completely broken.  The page cache
> is owned by the file system, so erofs can't simply poke into it.

The page cache is indeed owned by the underlay file system
instead, but erofs doesn't poke into it: it just needs some
temporary metadata read usage without extra allocated buffers.

On the one side, I hope if there could be some interface for
such temporary usage rather than just one vfs_iter_read model.

> 
> Now for reads it mostly works on the most common disk-based file systems,
> but it does create lots of problem for slightly more complex ones like
> network/clustered or synthetic file systems.  It also really breaks

Just out of curiousity, could you point out one specific path
so I can look into that.

> layering, so we need to fix it.  Not sure what would be best, but I'd be
> tempted to have a cross-instance cache maintained by erofs and filled
> using in-kernel direct I/O.  IFF the page policies work great for you

Direct I/O may be improper for many cases, since users will use
buffer I/Os to download the images from remotes just now, and
direct I/O just makes it worse (invalidate the cache, and reread
from disk) and double caching if underlay file is also read.

> that even could be a synthetic inode/mapping.

I expect the similar comments, if we really need to work out such
cross-instance cache, I'm fine to implement for Linux 7.2.  It will
increase the complexity of the codebase and also it won't share the
cache with the underlay fs.

But could we just fix this issue first for previous linux versions?

> 
>> 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}.
> 
> Note that this does not add the bypass, just extends it to both I/O
> types.  But yes, this breaks fanotify.  We actually have quite a few
> raw ->read_iter/->write_iter calls, so this might need more structured
> treatment.

It also bypasses the security hooks I think.

Thanks,
Gao Xiang

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

[Christoph Hellwig]

On Fri, May 08, 2026 at 05:10:21PM +0800, Gao Xiang wrote:
> On the one side, I hope if there could be some interface for
> such temporary usage rather than just one vfs_iter_read model.

As in a in-kernel mmap?  While not entirely impossible, the locking
model for that sounds horrible.

> > Now for reads it mostly works on the most common disk-based file systems,
> > but it does create lots of problem for slightly more complex ones like
> > network/clustered or synthetic file systems.  It also really breaks
> 
> Just out of curiousity, could you point out one specific path
> so I can look into that.

file system might require their own locking, e.g. cluster locks for
cluster file systems, and at least in the path direct page cache access
also caused problems with NFS data invalidation semantics.  Last but not
least ->read_folio has a file paramater that isn't really a file but a
file system specific cookie.  So calling this with something not managed
by the file system can cause problems as has caused crashes in the past,
although the offender at that time (the old smbfs) is now gone.

> But could we just fix this issue first for previous linux versions?

I just pointed out another issue.  You'll have to fix the credentials
either way.

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

[Gao Xiang]

On 2026/5/11 14:18, Christoph Hellwig wrote:
> On Fri, May 08, 2026 at 05:10:21PM +0800, Gao Xiang wrote:
>> On the one side, I hope if there could be some interface for
>> such temporary usage rather than just one vfs_iter_read model.
> 
> As in a in-kernel mmap?  While not entirely impossible, the locking
> model for that sounds horrible.

I don't think it needs a full in-kernel mmap, it just works on
some uptodate folios.

Which locking model? For page cache, it's expected that all folios
shouldn't clear uptodate randomly at any time.

At least for erofs use cases, we only care uptodate folios, no
matter if it's being invalidated/truncated or not (mapping == NULL).
Maybe it's not suitable for other stricter cases, but for immutable
fs models, that is enough and efficient.

> 
>>> Now for reads it mostly works on the most common disk-based file systems,
>>> but it does create lots of problem for slightly more complex ones like
>>> network/clustered or synthetic file systems.  It also really breaks
>>
>> Just out of curiousity, could you point out one specific path
>> so I can look into that.
> 
> file system might require their own locking, e.g. cluster locks for
> cluster file systems, and at least in the path direct page cache access
> also caused problems with NFS data invalidation semantics.  Last but not
> least ->read_folio has a file paramater that isn't really a file but a
> file system specific cookie.  So calling this with something not managed
> by the file system can cause problems as has caused crashes in the past,
> although the offender at that time (the old smbfs) is now gone.

file is indeed a cookie, but I did some research on the codebase,
and I've seen no odd cases other than a real "struct file *" anymore.

I agree such usage is kind of gray area, but I've seen no risk in
practice as long as the underlay fs supports proper ->read_folio
callback (and erofs restricts that.)

> 
>> But could we just fix this issue first for previous linux versions?
> 
> I just pointed out another issue.  You'll have to fix the credentials
> either way.

I really hope Matthew could give some opinion on this too, because
this way, the underlay cache can be directly used for temporary use,
and it should be a RO access and won't impact any fs-owned state.

Anyway, I could work out an alternative, but that makes the metadata
access less efficient.

Thanks,
Gao Xiang

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

[Christian Brauner]

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 <hch@infradead.org> 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.

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

[Gao Xiang]

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 <hch@infradead.org> 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