Re: [PATCH] erofs: fix metabuf leak in shared xattr initialization

[Gao Xiang]

On 2026/5/20 12:25, Gao Xiang wrote:
> Hi Jia,
>
> On 2026/5/20 11:42, Jia Zhu wrote:
> > erofs_init_inode_xattrs() uses a local metabuf while reading the inline
> > xattr header and the shared xattr id array.
> >
> > It currently drops that metabuf from some error paths and from the success
> > path, but the erofs_bread() failure while reading the shared xattr id array
> > goes straight to out_unlock.
> >
> > This became observable when file-backed metadata reads started calling
> > rw_verify_area() before reusing or dropping the current metabuf.  Before
> > that, the read_mapping_folio() failure path already dropped the old metabuf
> > before returning an error.

even it's exposable directly due to commit 307210c262a2
"erofs: verify metadata accesses for file-backed mounts")

I still hope we could mark it as an xattr implementaion
issue instead of a random consequence, we should
erofs_put_metabuf() as long as `erofs_buf` was
successfully used once.

> > Consolidate the local metabuf cleanup at out_unlock. erofs_put_metabuf()
> > is a no-op if no page has been acquired, and this keeps all paths after
> > taking EROFS_I_BL_XATTR_BIT covered by one cleanup site.
> >
> > Fixes: 307210c262a2 ("erofs: verify metadata accesses for file-backed mounts")
>
>
> Hmm, I don't think it's a correct "Fixes:", and
> the commit message should be fixed too.
>
> I think it's an issue due to missing erofs_put_metabuf()
> in the erofs_init_inode_xattrs() error path instead.
>
> I think
> Fixes: bb88e8da0025 ("erofs: use meta buffers for xattr operations")
>
> is a more proper "Fixes:".
>
> Thanks,
> Gao Xiang
>
> > Signed-off-by: Jia Zhu <zhujia.zj@xxxxxxxxxxxxx>
> > ---
> >   fs/erofs/xattr.c | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> > index 41e311019a251..df7ea019526d7 100644
> > --- a/fs/erofs/xattr.c
> > +++ b/fs/erofs/xattr.c
> > @@ -89,13 +89,11 @@ static int erofs_init_inode_xattrs(struct inode *inode)
> >           vi->xattr_isize - sizeof(struct erofs_xattr_ibody_header)) {
> >           erofs_err(sb, "invalid h_shared_count %u @ nid %llu",
> >                 vi->xattr_shared_count, vi->nid);
> > -        erofs_put_metabuf(&buf);
> >           ret = -EFSCORRUPTED;
> >           goto out_unlock;
> >       }
> >       vi->xattr_shared_xattrs = kmalloc_objs(uint, vi->xattr_shared_count);
> >       if (!vi->xattr_shared_xattrs) {
> > -        erofs_put_metabuf(&buf);
> >           ret = -ENOMEM;
> >           goto out_unlock;
> >       }
> > @@ -112,12 +110,12 @@ static int erofs_init_inode_xattrs(struct inode *inode)
> >           }
> >           vi->xattr_shared_xattrs[i] = le32_to_cpu(*xattr_id);
> >       }
> > -    erofs_put_metabuf(&buf);
> >       /* paired with smp_mb() at the beginning of the function. */
> >       smp_mb();
> >       set_bit(EROFS_I_EA_INITED_BIT, &vi->flags);
> >   out_unlock:
> > +    erofs_put_metabuf(&buf);
> >       clear_and_wake_up_bit(EROFS_I_BL_XATTR_BIT, &vi->flags);
> >       return ret;
> >   }