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

[David Sterba <dsterba@suse.cz>]

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).