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

[binglei wang]

Sorry, my webbrowser email client configs with something wrong. Thanks
for your reply.

I re-sent the patch from my Linux shell. I believe it's OK this time.


Gao Xiang <xiang@xxxxxxxxxx> 于2026年9月30日周三 15:52写道：
>
> 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);