Re: [PATCH 2/2] Btrfs: fix unprotected deletion from pending_chunks list

[Filipe Manana <fdmanana@kernel.org>]

On Mon, Jan 21, 2019 at 7:07 PM Alex Lyakas <alex@zadara.com> wrote:
>
> Hi Filipe,
>
> On Tue, Dec 2, 2014 at 8:08 PM Filipe Manana <fdmanana@suse.com> wrote:
> >
> > On block group remove if the corresponding extent map was on the
> > transaction->pending_chunks list, we were deleting the extent map
> > from that list, through remove_extent_mapping(), without any
> > synchronization with chunk allocation (which iterates that list
> > and adds new elements to it). Fix this by ensure that this is done
> > while the chunk mutex is held, since that's the mutex that protects
> > the list in the chunk allocation code path.
> >
> > This applies on top (depends on) of my previous patch titled:
> > "Btrfs: fix race between fs trimming and block group remove/allocation"
> >
> > But the issue in fact was already present before that change, it only
> > became easier to hit after Josef's 3.18 patch that added automatic
> > removal of empty block groups.
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >  fs/btrfs/extent-tree.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 17d429d..a7b81b4 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -9524,19 +9524,25 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
> >                 list_move_tail(&em->list, &root->fs_info->pinned_chunks);
> >         }
> >         spin_unlock(&block_group->lock);
> > -       unlock_chunks(root);
> >
> >         if (remove_em) {
> >                 struct extent_map_tree *em_tree;
> >
> >                 em_tree = &root->fs_info->mapping_tree.map_tree;
> >                 write_lock(&em_tree->lock);
> > +               /*
> > +                * The em might be in the pending_chunks list, so make sure the
> > +                * chunk mutex is locked, since remove_extent_mapping() will
> > +                * delete us from that list.
> > +                */
> >                 remove_extent_mapping(em_tree, em);
> >                 write_unlock(&em_tree->lock);
> If the "em" was in pending_chunks, it will be deleted from that list
> by "remove_extent_mapping". But it looks like in this case we also
> need to drop the extra ref on "em", which was held by pending_chunks
> list. I don't see it being done anywhere else. So we should check
> before the remove_extent_mapping() call whether "em" was in
> pending_chunks, and, if yes, drop the extra ref?

This was part of a large patch set that fixed multiple issues with
automatic removal of block groups.
Dropping the extent map reference was done on another patch of that patch set:

commit 495e64f4fe0363bc79fa0dfb41c271787e01b5c3
Author: Filipe Manana <fdmanana@suse.com>
Date:   Tue Dec 2 18:07:30 2014 +0000

    Btrfs: fix fs mapping extent map leak

Over 4 years ago....

>
> Thanks,
> Alex.
>
>
> >                 /* once for the tree */
> >                 free_extent_map(em);
> >         }
> >
> > +       unlock_chunks(root);
> > +
> >         btrfs_put_block_group(block_group);
> >         btrfs_put_block_group(block_group);
> >
> > --
> > 2.1.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html