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

[Mark Harmstone <mark@harmstone.com>]

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.