[v1] btrfs: fix memory leak of qgroup_list in btrfs_add_qgroup_relation

[PATCH fs/btrfs] btrfs: fix memory leak of qgroup_list in btrfs_add_qgroup_relation

Posted by Shardul Bankar 3 months, 2 weeks ago

When btrfs_add_qgroup_relation() is called with invalid qgroup levels
(src >= dst), the function returns -EINVAL directly without freeing the
preallocated qgroup_list structure passed by the caller. This causes a
memory leak because the caller unconditionally sets the pointer to NULL
after the call, preventing any cleanup.

The issue occurs because the level validation check happens before the
mutex is acquired and before any error handling path that would free
the prealloc pointer. On this early return, the cleanup code at the
'out' label (which includes kfree(prealloc)) is never reached.

In btrfs_ioctl_qgroup_assign(), the code pattern is:

    prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
    ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
    prealloc = NULL;  // Always set to NULL regardless of return value
    ...
    kfree(prealloc);  // This becomes kfree(NULL), does nothing

When the level check fails, 'prealloc' is never freed by either the
callee or the caller, resulting in a 64-byte memory leak per failed
operation. This can be triggered repeatedly by an unprivileged user
with access to a writable btrfs mount, potentially exhausting kernel
memory.

Fix this by changing the early return to a goto that reaches the
cleanup code, ensuring prealloc is always freed on all error paths.

Reported-by: BRF (btrfs runtime fuzzer)
Fixes: 8465ecec9611 ("btrfs: Check qgroup level in kernel qgroup assign.")
Signed-off-by: Shardul Bankar <shardulsb08@gmail.com>
---
 fs/btrfs/qgroup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 1175b8192cd7..0a25bfdd442f 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1539,8 +1539,10 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
 	ASSERT(prealloc);
 
 	/* Check the level of src and dst first */
-	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
-		return -EINVAL;
+	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
-- 
2.34.1

Re: [PATCH fs/btrfs] btrfs: fix memory leak of qgroup_list in btrfs_add_qgroup_relation

Posted by Qu Wenruo 3 months, 2 weeks ago


在 2025/10/25 19:59, Shardul Bankar 写道:
> When btrfs_add_qgroup_relation() is called with invalid qgroup levels
> (src >= dst), the function returns -EINVAL directly without freeing the
> preallocated qgroup_list structure passed by the caller. This causes a
> memory leak because the caller unconditionally sets the pointer to NULL
> after the call, preventing any cleanup.
> 
> The issue occurs because the level validation check happens before the
> mutex is acquired and before any error handling path that would free
> the prealloc pointer. On this early return, the cleanup code at the
> 'out' label (which includes kfree(prealloc)) is never reached.
> 
> In btrfs_ioctl_qgroup_assign(), the code pattern is:
> 
>      prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
>      ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
>      prealloc = NULL;  // Always set to NULL regardless of return value
>      ...
>      kfree(prealloc);  // This becomes kfree(NULL), does nothing
> 
> When the level check fails, 'prealloc' is never freed by either the
> callee or the caller, resulting in a 64-byte memory leak per failed
> operation. This can be triggered repeatedly by an unprivileged user
> with access to a writable btrfs mount, potentially exhausting kernel
> memory.
> 
> Fix this by changing the early return to a goto that reaches the
> cleanup code, ensuring prealloc is always freed on all error paths.
> 
> Reported-by: BRF (btrfs runtime fuzzer)
> Fixes: 8465ecec9611 ("btrfs: Check qgroup level in kernel qgroup assign.")
> Signed-off-by: Shardul Bankar <shardulsb08@gmail.com>
> ---
>   fs/btrfs/qgroup.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 1175b8192cd7..0a25bfdd442f 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1539,8 +1539,10 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
>   	ASSERT(prealloc);
>   
>   	/* Check the level of src and dst first */
> -	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
> -		return -EINVAL;
> +	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

Out will call mutex_unlock(), but we haven't yet even locked the mutex.

Please just call kfree(prealloc) then return.

Thanks,
Qu

>   
>   	mutex_lock(&fs_info->qgroup_ioctl_lock);
>   	if (!fs_info->quota_root) {

Re: [PATCH fs/btrfs] btrfs: fix memory leak of qgroup_list in btrfs_add_qgroup_relation

Posted by Qu Wenruo 3 months, 2 weeks ago


在 2025/10/25 20:36, Qu Wenruo 写道:
> 
> 
> 在 2025/10/25 19:59, Shardul Bankar 写道:
>> When btrfs_add_qgroup_relation() is called with invalid qgroup levels
>> (src >= dst), the function returns -EINVAL directly without freeing the
>> preallocated qgroup_list structure passed by the caller. This causes a
>> memory leak because the caller unconditionally sets the pointer to NULL
>> after the call, preventing any cleanup.
>>
>> The issue occurs because the level validation check happens before the
>> mutex is acquired and before any error handling path that would free
>> the prealloc pointer. On this early return, the cleanup code at the
>> 'out' label (which includes kfree(prealloc)) is never reached.
>>
>> In btrfs_ioctl_qgroup_assign(), the code pattern is:
>>
>>      prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
>>      ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
>>      prealloc = NULL;  // Always set to NULL regardless of return value
>>      ...
>>      kfree(prealloc);  // This becomes kfree(NULL), does nothing
>>
>> When the level check fails, 'prealloc' is never freed by either the
>> callee or the caller, resulting in a 64-byte memory leak per failed
>> operation. This can be triggered repeatedly by an unprivileged user
>> with access to a writable btrfs mount, potentially exhausting kernel
>> memory.
>>
>> Fix this by changing the early return to a goto that reaches the
>> cleanup code, ensuring prealloc is always freed on all error paths.
>>
>> Reported-by: BRF (btrfs runtime fuzzer)

And forgot to mention, if this is some tool that has sent a report to 
the mailing list, please add a "Link:" tag.

If it's only something internal to your organization/project, please 
remove this line, it makes no sense for upstream.

Thanks,
Qu

>> Fixes: 8465ecec9611 ("btrfs: Check qgroup level in kernel qgroup 
>> assign.")
>> Signed-off-by: Shardul Bankar <shardulsb08@gmail.com>
>> ---
>>   fs/btrfs/qgroup.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 1175b8192cd7..0a25bfdd442f 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1539,8 +1539,10 @@ int btrfs_add_qgroup_relation(struct 
>> btrfs_trans_handle *trans, u64 src, u64 dst
>>       ASSERT(prealloc);
>>       /* Check the level of src and dst first */
>> -    if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
>> -        return -EINVAL;
>> +    if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst)) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
> 
> Out will call mutex_unlock(), but we haven't yet even locked the mutex.
> 
> Please just call kfree(prealloc) then return.
> 
> Thanks,
> Qu
> 
>>       mutex_lock(&fs_info->qgroup_ioctl_lock);
>>       if (!fs_info->quota_root) {
> 
>

[PATCH v3 fs/btrfs v3] btrfs: fix memory leak of qgroup_list in btrfs_add_qgroup_relation

Posted by Shardul Bankar 3 months, 2 weeks ago

When btrfs_add_qgroup_relation() is called with invalid qgroup levels
(src >= dst), the function returns -EINVAL directly without freeing the
preallocated qgroup_list structure passed by the caller. This causes a
memory leak because the caller unconditionally sets the pointer to NULL
after the call, preventing any cleanup.

The issue occurs because the level validation check happens before the
mutex is acquired and before any error handling path that would free
the prealloc pointer. On this early return, the cleanup code at the
'out' label (which includes kfree(prealloc)) is never reached.

In btrfs_ioctl_qgroup_assign(), the code pattern is:

    prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
    ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
    prealloc = NULL;  // Always set to NULL regardless of return value
    ...
    kfree(prealloc);  // This becomes kfree(NULL), does nothing

When the level check fails, 'prealloc' is never freed by either the
callee or the caller, resulting in a 64-byte memory leak per failed
operation. This can be triggered repeatedly by an unprivileged user
with access to a writable btrfs mount, potentially exhausting kernel
memory.

Fix this by freeing prealloc before the early return, ensuring prealloc
is always freed on all error paths.

Fixes: 4addc1ffd67a ("btrfs: qgroup: preallocate memory before adding a relation")
Signed-off-by: Shardul Bankar <shardulsb08@gmail.com>
---

v3:
 - Update Fixes: tag to correct commit SHA as suggested by Filipe Manana.
 - No code changes.

v2:
 - Free prealloc directly before returning -EINVAL (no mutex held),
   per review from Qu Wenruo.
 - Drop goto-based cleanup.

 fs/btrfs/qgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 1175b8192cd7..31ad8580322a 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1539,8 +1539,10 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
 	ASSERT(prealloc);
 
 	/* Check the level of src and dst first */
-	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
+	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst)) {
+		kfree(prealloc);
 		return -EINVAL;
+	}
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
-- 
2.34.1

Re: [PATCH v3 fs/btrfs v3] btrfs: fix memory leak of qgroup_list in btrfs_add_qgroup_relation

Posted by Qu Wenruo 3 months, 1 week ago


在 2025/10/26 06:30, Shardul Bankar 写道:

You don't need to include the "fs/btrfs v3" in the [PATCH] part.

You can refer to the mailing list for how to add versions to the patches.
Thankfully the "fs/btrfs v3" part will just be discarded by git-am.

> When btrfs_add_qgroup_relation() is called with invalid qgroup levels
> (src >= dst), the function returns -EINVAL directly without freeing the
> preallocated qgroup_list structure passed by the caller. This causes a
> memory leak because the caller unconditionally sets the pointer to NULL
> after the call, preventing any cleanup.
> 
> The issue occurs because the level validation check happens before the
> mutex is acquired and before any error handling path that would free
> the prealloc pointer. On this early return, the cleanup code at the
> 'out' label (which includes kfree(prealloc)) is never reached.
> 
> In btrfs_ioctl_qgroup_assign(), the code pattern is:
> 
>      prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
>      ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
>      prealloc = NULL;  // Always set to NULL regardless of return value
>      ...
>      kfree(prealloc);  // This becomes kfree(NULL), does nothing
> 
> When the level check fails, 'prealloc' is never freed by either the
> callee or the caller, resulting in a 64-byte memory leak per failed
> operation. This can be triggered repeatedly by an unprivileged user
> with access to a writable btrfs mount, potentially exhausting kernel
> memory.
> 
> Fix this by freeing prealloc before the early return, ensuring prealloc
> is always freed on all error paths.
> 
> Fixes: 4addc1ffd67a ("btrfs: qgroup: preallocate memory before adding a relation")
> Signed-off-by: Shardul Bankar <shardulsb08@gmail.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

And now pushed to for-next branch.

Thanks,
Qu

> ---
> 
> v3:
>   - Update Fixes: tag to correct commit SHA as suggested by Filipe Manana.
>   - No code changes.
> 
> v2:
>   - Free prealloc directly before returning -EINVAL (no mutex held),
>     per review from Qu Wenruo.
>   - Drop goto-based cleanup.
> 
>   fs/btrfs/qgroup.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 1175b8192cd7..31ad8580322a 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1539,8 +1539,10 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
>   	ASSERT(prealloc);
>   
>   	/* Check the level of src and dst first */
> -	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
> +	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst)) {
> +		kfree(prealloc);
>   		return -EINVAL;
> +	}
>   
>   	mutex_lock(&fs_info->qgroup_ioctl_lock);
>   	if (!fs_info->quota_root) {

[PATCH v2 fs/btrfs v2] btrfs: fix memory leak of qgroup_list in btrfs_add_qgroup_relation

Posted by Shardul Bankar 3 months, 2 weeks ago

When btrfs_add_qgroup_relation() is called with invalid qgroup levels
(src >= dst), the function returns -EINVAL directly without freeing the
preallocated qgroup_list structure passed by the caller. This causes a
memory leak because the caller unconditionally sets the pointer to NULL
after the call, preventing any cleanup.

The issue occurs because the level validation check happens before the
mutex is acquired and before any error handling path that would free
the prealloc pointer. On this early return, the cleanup code at the
'out' label (which includes kfree(prealloc)) is never reached.

In btrfs_ioctl_qgroup_assign(), the code pattern is:

    prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
    ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
    prealloc = NULL;  // Always set to NULL regardless of return value
    ...
    kfree(prealloc);  // This becomes kfree(NULL), does nothing

When the level check fails, 'prealloc' is never freed by either the
callee or the caller, resulting in a 64-byte memory leak per failed
operation. This can be triggered repeatedly by an unprivileged user
with access to a writable btrfs mount, potentially exhausting kernel
memory.

Fix this by freeing prealloc before the early return, ensuring prealloc
is always freed on all error paths.

Fixes: 8465ecec9611 ("btrfs: Check qgroup level in kernel qgroup assign.")
Cc: stable@vger.kernel.org # v4.0+
Signed-off-by: Shardul Bankar <shardulsb08@gmail.com>
---

v2:
 - Free prealloc directly before returning -EINVAL (no mutex held),
   per review from Qu Wenruo.
 - Drop goto-based cleanup.

 fs/btrfs/qgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 1175b8192cd7..31ad8580322a 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1539,8 +1539,10 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
 	ASSERT(prealloc);
 
 	/* Check the level of src and dst first */
-	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
+	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst)) {
+		kfree(prealloc);
 		return -EINVAL;
+	}
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
-- 
2.34.1

Re: [PATCH v2 fs/btrfs v2] btrfs: fix memory leak of qgroup_list in btrfs_add_qgroup_relation

Posted by Filipe Manana 3 months, 2 weeks ago

On Sat, Oct 25, 2025 at 1:05 PM Shardul Bankar <shardulsb08@gmail.com> wrote:
>
> When btrfs_add_qgroup_relation() is called with invalid qgroup levels
> (src >= dst), the function returns -EINVAL directly without freeing the
> preallocated qgroup_list structure passed by the caller. This causes a
> memory leak because the caller unconditionally sets the pointer to NULL
> after the call, preventing any cleanup.
>
> The issue occurs because the level validation check happens before the
> mutex is acquired and before any error handling path that would free
> the prealloc pointer. On this early return, the cleanup code at the
> 'out' label (which includes kfree(prealloc)) is never reached.
>
> In btrfs_ioctl_qgroup_assign(), the code pattern is:
>
>     prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
>     ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
>     prealloc = NULL;  // Always set to NULL regardless of return value
>     ...
>     kfree(prealloc);  // This becomes kfree(NULL), does nothing
>
> When the level check fails, 'prealloc' is never freed by either the
> callee or the caller, resulting in a 64-byte memory leak per failed
> operation. This can be triggered repeatedly by an unprivileged user
> with access to a writable btrfs mount, potentially exhausting kernel
> memory.
>
> Fix this by freeing prealloc before the early return, ensuring prealloc
> is always freed on all error paths.
>
> Fixes: 8465ecec9611 ("btrfs: Check qgroup level in kernel qgroup assign.")

This is completely wrong...
When that commit landed we didn't even have the 'prealloc'...

The right commit is:

4addc1ffd67a ("btrfs: qgroup: preallocate memory before adding a relation")

> Cc: stable@vger.kernel.org # v4.0+

And this becomes 6.11. But with a Fixes tag we pretty much don't need
it, stable scripts will figure everything out.

Thanks.


> Signed-off-by: Shardul Bankar <shardulsb08@gmail.com>
> ---
>
> v2:
>  - Free prealloc directly before returning -EINVAL (no mutex held),
>    per review from Qu Wenruo.
>  - Drop goto-based cleanup.
>
>  fs/btrfs/qgroup.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 1175b8192cd7..31ad8580322a 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1539,8 +1539,10 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
>         ASSERT(prealloc);
>
>         /* Check the level of src and dst first */
> -       if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
> +       if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst)) {
> +               kfree(prealloc);
>                 return -EINVAL;
> +       }
>
>         mutex_lock(&fs_info->qgroup_ioctl_lock);
>         if (!fs_info->quota_root) {
> --
> 2.34.1
>
>