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

[Gao Xiang <hsiangkao@linux.alibaba.com>]

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@bytedance.com>
>> ---
>>   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;
>>   }
>