erofs: fix metabuf leak in shared xattr initialization

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

Posted by Jia Zhu 4 days, 20 hours ago

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.

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")
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;
 }
-- 
2.39.5 (Apple Git-154)

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

Posted by Gao Xiang 4 days, 19 hours ago

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

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

Posted by Gao Xiang 4 days, 19 hours ago


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

[PATCH v2] erofs: fix metabuf leak in inode xattr initialization

Posted by Jia Zhu 4 days, 19 hours ago

commit bb88e8da0025 ("erofs: use meta buffers for xattr operations")
converted xattr operations to use on-stack erofs_buf instances.
erofs_init_inode_xattrs() uses such a metabuf while reading the inline
xattr header and shared xattr id array.

Some error paths after erofs_read_metabuf() leave through out_unlock
without dropping the metabuf, so the folio reference can leak.

Consolidate the cleanup at out_unlock. erofs_put_metabuf() is a
no-op if no folio has been acquired, and this keeps all paths after
taking EROFS_I_BL_XATTR_BIT covered by a single cleanup site.

Fixes: bb88e8da0025 ("erofs: use meta buffers for xattr operations")

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;
 }
-- 
2.39.5 (Apple Git-154)

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

Posted by Gao Xiang 4 days, 17 hours ago


On 2026/5/20 12:46, Jia Zhu wrote:
> commit bb88e8da0025 ("erofs: use meta buffers for xattr operations")
> converted xattr operations to use on-stack erofs_buf instances.
> erofs_init_inode_xattrs() uses such a metabuf while reading the inline
> xattr header and shared xattr id array.
> 
> Some error paths after erofs_read_metabuf() leave through out_unlock
> without dropping the metabuf, so the folio reference can leak.
> 
> Consolidate the cleanup at out_unlock. erofs_put_metabuf() is a
> no-op if no folio has been acquired, and this keeps all paths after
> taking EROFS_I_BL_XATTR_BIT covered by a single cleanup site.
> 
> Fixes: bb88e8da0025 ("erofs: use meta buffers for xattr operations")
> 

Useless new line, I will remove this line manually when applying.

> Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang