[PATCH] btrfs: fix potential segfault in balance_remap_chunks()

[PATCH] btrfs: fix potential segfault in balance_remap_chunks()

[Mark Harmstone]

Fix a potential segfault in balance_remap_chunks(): if we quit early
because btrfs_inc_block_group_ro() fails, all the remaining items in the
chunks list will still have their bg value set to NULL. It's thus not
safe to dereference this pointer without checking first.

Link: https://lore.kernel.org/linux-btrfs/20260125120717.1578828-1-clm@meta.com/
Reported-by: Chris Mason <clm@fb.com>
Fixes: 81e5a4551c32 ("btrfs: allow balancing remap tree")
Signed-off-by: Mark Harmstone <mark@harmstone.com>
---
 fs/btrfs/volumes.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e15e138c515b..18911cdd2895 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4288,17 +4288,19 @@ static int balance_remap_chunks(struct btrfs_fs_info *fs_info, struct btrfs_path
 
 		rci = list_first_entry(chunks, struct remap_chunk_info, list);
 
-		spin_lock(&rci->bg->lock);
-		is_unused = !btrfs_is_block_group_used(rci->bg);
-		spin_unlock(&rci->bg->lock);
+		if (rci->bg) {
+			spin_lock(&rci->bg->lock);
+			is_unused = !btrfs_is_block_group_used(rci->bg);
+			spin_unlock(&rci->bg->lock);
 
-		if (is_unused)
-			btrfs_mark_bg_unused(rci->bg);
+			if (is_unused)
+				btrfs_mark_bg_unused(rci->bg);
 
-		if (rci->made_ro)
-			btrfs_dec_block_group_ro(rci->bg);
+			if (rci->made_ro)
+				btrfs_dec_block_group_ro(rci->bg);
 
-		btrfs_put_block_group(rci->bg);
+			btrfs_put_block_group(rci->bg);
+		}
 
 		list_del(&rci->list);
 		kfree(rci);
-- 
2.52.0

Re: [PATCH] btrfs: fix potential segfault in balance_remap_chunks()

[Johannes Thumshirn]

On 2/25/26 11:36 AM, Mark Harmstone wrote:
> Fix a potential segfault in balance_remap_chunks(): if we quit early
> because btrfs_inc_block_group_ro() fails, all the remaining items in the
> chunks list will still have their bg value set to NULL. It's thus not
> safe to dereference this pointer without checking first.
>
> Link: https://lore.kernel.org/linux-btrfs/20260125120717.1578828-1-clm@meta.com/
> Reported-by: Chris Mason <clm@fb.com>
> Fixes: 81e5a4551c32 ("btrfs: allow balancing remap tree")
> Signed-off-by: Mark Harmstone <mark@harmstone.com>
> ---
>   fs/btrfs/volumes.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e15e138c515b..18911cdd2895 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4288,17 +4288,19 @@ static int balance_remap_chunks(struct btrfs_fs_info *fs_info, struct btrfs_path
>   
>   		rci = list_first_entry(chunks, struct remap_chunk_info, list);
>   
> -		spin_lock(&rci->bg->lock);
> -		is_unused = !btrfs_is_block_group_used(rci->bg);
> -		spin_unlock(&rci->bg->lock);
> +		if (rci->bg) {
> +			spin_lock(&rci->bg->lock);
> +			is_unused = !btrfs_is_block_group_used(rci->bg);
> +			spin_unlock(&rci->bg->lock);
>   
> -		if (is_unused)
> -			btrfs_mark_bg_unused(rci->bg);
> +			if (is_unused)
> +				btrfs_mark_bg_unused(rci->bg);
>   
> -		if (rci->made_ro)
> -			btrfs_dec_block_group_ro(rci->bg);
> +			if (rci->made_ro)
> +				btrfs_dec_block_group_ro(rci->bg);
>   
> -		btrfs_put_block_group(rci->bg);
> +			btrfs_put_block_group(rci->bg);
> +		}
>   
>   		list_del(&rci->list);
>   		kfree(rci);

Not necessarily a hot path here, but this could be made way more 
readable if you'd have a local bg variable, sth like this:

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e15e138c515b..bf315625890d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4285,20 +4285,24 @@ static int balance_remap_chunks(struct 
btrfs_fs_info *fs_info, struct btrfs_path
  end:
      while (!list_empty(chunks)) {
          bool is_unused;
+        struct btrfs_block_group *bg;

          rci = list_first_entry(chunks, struct remap_chunk_info, list);

-        spin_lock(&rci->bg->lock);
-        is_unused = !btrfs_is_block_group_used(rci->bg);
-        spin_unlock(&rci->bg->lock);
+        bg = rci->bg;
+        if (bg) {
+            spin_lock(&bg->lock);
+            is_unused = !btrfs_is_block_group_used(bg);
+            spin_unlock(&bg->lock);

-        if (is_unused)
-            btrfs_mark_bg_unused(rci->bg);
+            if (is_unused)
+                btrfs_mark_bg_unused(bg);

-        if (rci->made_ro)
-            btrfs_dec_block_group_ro(rci->bg);
+            if (rci->made_ro)
+                btrfs_dec_block_group_ro(bg);

-        btrfs_put_block_group(rci->bg);
+            btrfs_put_block_group(bg);
+        }

          list_del(&rci->list);
          kfree(rci);

Re: [PATCH] btrfs: fix potential segfault in balance_remap_chunks()

[David Sterba]

On Wed, Feb 25, 2026 at 10:35:31AM +0000, Mark Harmstone wrote:
> Fix a potential segfault in balance_remap_chunks(): if we quit early
> because btrfs_inc_block_group_ro() fails, all the remaining items in the
> chunks list will still have their bg value set to NULL. It's thus not
> safe to dereference this pointer without checking first.
> 
> Link: https://lore.kernel.org/linux-btrfs/20260125120717.1578828-1-clm@meta.com/
> Reported-by: Chris Mason <clm@fb.com>
> Fixes: 81e5a4551c32 ("btrfs: allow balancing remap tree")
> Signed-off-by: Mark Harmstone <mark@harmstone.com>
> ---
>  fs/btrfs/volumes.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e15e138c515b..18911cdd2895 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4288,17 +4288,19 @@ static int balance_remap_chunks(struct btrfs_fs_info *fs_info, struct btrfs_path
>  
>  		rci = list_first_entry(chunks, struct remap_chunk_info, list);
>  
> -		spin_lock(&rci->bg->lock);
> -		is_unused = !btrfs_is_block_group_used(rci->bg);
> -		spin_unlock(&rci->bg->lock);
> +		if (rci->bg) {
> +			spin_lock(&rci->bg->lock);
> +			is_unused = !btrfs_is_block_group_used(rci->bg);
> +			spin_unlock(&rci->bg->lock);
>  
> -		if (is_unused)
> -			btrfs_mark_bg_unused(rci->bg);

Not related to the patch but isn't this pattern inherently racy?

The "used" is read under lock but then btrfs_mark_bg_unused() is outside
of the lock so the status can change. This can be seen it more places,
but they seem to be related to the remap tree feature.

Re: [PATCH] btrfs: fix potential segfault in balance_remap_chunks()

[Johannes Thumshirn]

On 2/25/26 4:10 PM, David Sterba wrote:
> On Wed, Feb 25, 2026 at 10:35:31AM +0000, Mark Harmstone wrote:
>> Fix a potential segfault in balance_remap_chunks(): if we quit early
>> because btrfs_inc_block_group_ro() fails, all the remaining items in the
>> chunks list will still have their bg value set to NULL. It's thus not
>> safe to dereference this pointer without checking first.
>>
>> Link: https://lore.kernel.org/linux-btrfs/20260125120717.1578828-1-clm@meta.com/
>> Reported-by: Chris Mason <clm@fb.com>
>> Fixes: 81e5a4551c32 ("btrfs: allow balancing remap tree")
>> Signed-off-by: Mark Harmstone <mark@harmstone.com>
>> ---
>>   fs/btrfs/volumes.c | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index e15e138c515b..18911cdd2895 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -4288,17 +4288,19 @@ static int balance_remap_chunks(struct btrfs_fs_info *fs_info, struct btrfs_path
>>   
>>   		rci = list_first_entry(chunks, struct remap_chunk_info, list);
>>   
>> -		spin_lock(&rci->bg->lock);
>> -		is_unused = !btrfs_is_block_group_used(rci->bg);
>> -		spin_unlock(&rci->bg->lock);
>> +		if (rci->bg) {
>> +			spin_lock(&rci->bg->lock);
>> +			is_unused = !btrfs_is_block_group_used(rci->bg);
>> +			spin_unlock(&rci->bg->lock);
>>   
>> -		if (is_unused)
>> -			btrfs_mark_bg_unused(rci->bg);
> Not related to the patch but isn't this pattern inherently racy?
>
> The "used" is read under lock but then btrfs_mark_bg_unused() is outside
> of the lock so the status can change. This can be seen it more places,
> but they seem to be related to the remap tree feature.

Good catch, it is! But I /think/ it's not something to worry about (too 
much), because btrfs_delete_unused_bgs() will do checks on it's own as 
well, so if the "unused" state changes, nothing bad will happen.

Re: [PATCH] btrfs: fix potential segfault in balance_remap_chunks()

[Filipe Manana]

On Wed, Feb 25, 2026 at 3:10 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Feb 25, 2026 at 10:35:31AM +0000, Mark Harmstone wrote:
> > Fix a potential segfault in balance_remap_chunks(): if we quit early
> > because btrfs_inc_block_group_ro() fails, all the remaining items in the
> > chunks list will still have their bg value set to NULL. It's thus not
> > safe to dereference this pointer without checking first.
> >
> > Link: https://lore.kernel.org/linux-btrfs/20260125120717.1578828-1-clm@meta.com/
> > Reported-by: Chris Mason <clm@fb.com>
> > Fixes: 81e5a4551c32 ("btrfs: allow balancing remap tree")
> > Signed-off-by: Mark Harmstone <mark@harmstone.com>
> > ---
> >  fs/btrfs/volumes.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index e15e138c515b..18911cdd2895 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -4288,17 +4288,19 @@ static int balance_remap_chunks(struct btrfs_fs_info *fs_info, struct btrfs_path
> >
> >               rci = list_first_entry(chunks, struct remap_chunk_info, list);
> >
> > -             spin_lock(&rci->bg->lock);
> > -             is_unused = !btrfs_is_block_group_used(rci->bg);
> > -             spin_unlock(&rci->bg->lock);
> > +             if (rci->bg) {
> > +                     spin_lock(&rci->bg->lock);
> > +                     is_unused = !btrfs_is_block_group_used(rci->bg);
> > +                     spin_unlock(&rci->bg->lock);
> >
> > -             if (is_unused)
> > -                     btrfs_mark_bg_unused(rci->bg);
>
> Not related to the patch but isn't this pattern inherently racy?
>
> The "used" is read under lock but then btrfs_mark_bg_unused() is outside
> of the lock so the status can change. This can be seen it more places,
> but they seem to be related to the remap tree feature.

It's not a problem since when attempting to delete unused bgs we skip
any if they are actually used.
It's done not just for this type of race but also for the case where
after added to the unused list, it becomes used before the cleaner
kthread runs to delete unused bgs.

>

Re: [PATCH] btrfs: fix potential segfault in balance_remap_chunks()

[Mark Harmstone]

On 25/02/2026 3.17 pm, Filipe Manana wrote:
> On Wed, Feb 25, 2026 at 3:10 PM David Sterba <dsterba@suse.cz> wrote:
>>
>> On Wed, Feb 25, 2026 at 10:35:31AM +0000, Mark Harmstone wrote:
>>> Fix a potential segfault in balance_remap_chunks(): if we quit early
>>> because btrfs_inc_block_group_ro() fails, all the remaining items in the
>>> chunks list will still have their bg value set to NULL. It's thus not
>>> safe to dereference this pointer without checking first.
>>>
>>> Link: https://lore.kernel.org/linux-btrfs/20260125120717.1578828-1-clm@meta.com/
>>> Reported-by: Chris Mason <clm@fb.com>
>>> Fixes: 81e5a4551c32 ("btrfs: allow balancing remap tree")
>>> Signed-off-by: Mark Harmstone <mark@harmstone.com>
>>> ---
>>>   fs/btrfs/volumes.c | 18 ++++++++++--------
>>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index e15e138c515b..18911cdd2895 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -4288,17 +4288,19 @@ static int balance_remap_chunks(struct btrfs_fs_info *fs_info, struct btrfs_path
>>>
>>>                rci = list_first_entry(chunks, struct remap_chunk_info, list);
>>>
>>> -             spin_lock(&rci->bg->lock);
>>> -             is_unused = !btrfs_is_block_group_used(rci->bg);
>>> -             spin_unlock(&rci->bg->lock);
>>> +             if (rci->bg) {
>>> +                     spin_lock(&rci->bg->lock);
>>> +                     is_unused = !btrfs_is_block_group_used(rci->bg);
>>> +                     spin_unlock(&rci->bg->lock);
>>>
>>> -             if (is_unused)
>>> -                     btrfs_mark_bg_unused(rci->bg);
>>
>> Not related to the patch but isn't this pattern inherently racy?
>>
>> The "used" is read under lock but then btrfs_mark_bg_unused() is outside
>> of the lock so the status can change. This can be seen it more places,
>> but they seem to be related to the remap tree feature.
> 
> It's not a problem since when attempting to delete unused bgs we skip
> any if they are actually used.
> It's done not just for this type of race but also for the case where
> after added to the unused list, it becomes used before the cleaner
> kthread runs to delete unused bgs.

Yes, the unused list means less "these BGs are definitely unused", more 
"these BGs are worth considering as maybe unused".

Johannes, your suggestion that we add a bg variable is a good one - I'll 
send another patch doing that.

Re: [PATCH] btrfs: fix potential segfault in balance_remap_chunks()

[David Sterba]

On Mon, Mar 09, 2026 at 06:00:30PM +0000, Mark Harmstone wrote:
> On 25/02/2026 3.17 pm, Filipe Manana wrote:
> > On Wed, Feb 25, 2026 at 3:10 PM David Sterba <dsterba@suse.cz> wrote:
> >>
> >> On Wed, Feb 25, 2026 at 10:35:31AM +0000, Mark Harmstone wrote:
> >>> Fix a potential segfault in balance_remap_chunks(): if we quit early
> >>> because btrfs_inc_block_group_ro() fails, all the remaining items in the
> >>> chunks list will still have their bg value set to NULL. It's thus not
> >>> safe to dereference this pointer without checking first.
> >>>
> >>> Link: https://lore.kernel.org/linux-btrfs/20260125120717.1578828-1-clm@meta.com/
> >>> Reported-by: Chris Mason <clm@fb.com>
> >>> Fixes: 81e5a4551c32 ("btrfs: allow balancing remap tree")
> >>> Signed-off-by: Mark Harmstone <mark@harmstone.com>
> >>> ---
> >>>   fs/btrfs/volumes.c | 18 ++++++++++--------
> >>>   1 file changed, 10 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >>> index e15e138c515b..18911cdd2895 100644
> >>> --- a/fs/btrfs/volumes.c
> >>> +++ b/fs/btrfs/volumes.c
> >>> @@ -4288,17 +4288,19 @@ static int balance_remap_chunks(struct btrfs_fs_info *fs_info, struct btrfs_path
> >>>
> >>>                rci = list_first_entry(chunks, struct remap_chunk_info, list);
> >>>
> >>> -             spin_lock(&rci->bg->lock);
> >>> -             is_unused = !btrfs_is_block_group_used(rci->bg);
> >>> -             spin_unlock(&rci->bg->lock);
> >>> +             if (rci->bg) {
> >>> +                     spin_lock(&rci->bg->lock);
> >>> +                     is_unused = !btrfs_is_block_group_used(rci->bg);
> >>> +                     spin_unlock(&rci->bg->lock);
> >>>
> >>> -             if (is_unused)
> >>> -                     btrfs_mark_bg_unused(rci->bg);
> >>
> >> Not related to the patch but isn't this pattern inherently racy?
> >>
> >> The "used" is read under lock but then btrfs_mark_bg_unused() is outside
> >> of the lock so the status can change. This can be seen it more places,
> >> but they seem to be related to the remap tree feature.
> > 
> > It's not a problem since when attempting to delete unused bgs we skip
> > any if they are actually used.
> > It's done not just for this type of race but also for the case where
> > after added to the unused list, it becomes used before the cleaner
> > kthread runs to delete unused bgs.
> 
> Yes, the unused list means less "these BGs are definitely unused", more 
> "these BGs are worth considering as maybe unused".

So if there's a question or need for clarification in a patch thread
please add it as a comment (commit updated).