Re: [PATCH] erofs: fix folio reuse from a different address_space in erofs_bread()

[Gao Xiang]

On Wed, Sep 30, 2026 at 11:37:53AM +0800, binglei wang wrote:
> erofs_bread() caches the last folio in struct erofs_buf and reuses it when
> the next request lands on the same folio, but the reuse predicate only
> compares the page index; it never checks that the cached folio still
> belongs to buf->mapping.  Correctness therefore relies on an invariant
> that is nowhere enforced.
> 
> fs/erofs/xattr.c already breaks it: erofs_xattr_iter_inline() and
> erofs_xattr_iter_shared() call erofs_init_metabuf() on the same buffer
> with no intervening erofs_put_metabuf(), and their in_metabox arguments
> come from different sources (per-inode vs per-fs).  When the two differ,
> buf->mapping is switched while buf->page still holds a folio of the
> previous address_space, so the next erofs_bread() can return data from
> the wrong one.
> 
> This is observable with METABOX enabled, where shared xattrs silently
> disappear on the mounted fs, while the same tree built without METABOX
> reports them fine.
> 
> Fix it by validating the address_space in the reuse predicate too: if the
> cached folio belongs to another mapping, drop it so that the existing
> slow path re-reads from buf->mapping.  Reading folio->mapping is safe
> here because a reference on the cached folio is still held; if the folio
> was already truncated, folio->mapping is NULL and the slow path is taken,
> which is the safe direction.
> 
> Fixes: 414091322c63 ("erofs: implement metadata compression")
> Cc: Bo Liu (OpenAnolis) <liubo03@xxxxxxxxxx>
> Signed-off-by: Binglei Wang <l3b2w1@xxxxxxxxx>

Thanks for the patch.

 1) The patch format is still broken as your previous patch, please
    check your email client again before sending out a new patch
    (as I said, you could send a patch to yourself and try to apply
     the patch and see if it works);

 2) the commit message of this patch is too over long (mostly
in the LLM-generated style), I think you could simplify a bit since
it's more friendly to human developers.

> ---
>  fs/erofs/data.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index be63b89f0862..d8b6523bc218 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -33,8 +33,13 @@ void *erofs_bread(struct erofs_buf *buf,
> erofs_off_t offset, bool need_kmap)
> 
>   if (buf->page) {
>   folio = page_folio(buf->page);
> - if (folio_file_page(folio, index) != buf->page)
> + if (folio->mapping != buf->mapping) {
> + /* the cached folio belongs to another address_space */
> + erofs_put_metabuf(buf);
> + folio = NULL;

     `folio = NULL` is enough?

Thanks,
Gao Xiang

> + } else if (folio_file_page(folio, index) != buf->page) {
>   erofs_unmap_metabuf(buf);
> + }
>   }
>   if (!folio || !folio_contains(folio, index)) {
>   erofs_put_metabuf(buf);