Re: [PATCH] btrfs: free the allocated memory if btrfs_alloc_page_array() failed

[David Sterba <dsterba@suse.cz>]

On Fri, Nov 24, 2023 at 12:47:37PM +0000, Filipe Manana wrote:
> On Fri, Nov 24, 2023 at 11:50 AM Filipe Manana <fdmanana@kernel.org> wrote:
> >
> > On Fri, Nov 24, 2023 at 4:30 AM Qu Wenruo <wqu@suse.com> wrote:
> > >
> > > [BUG]
> > > If btrfs_alloc_page_array() failed to allocate all pages but part of the
> > > slots, then the partially allocated pages would be leaked in function
> > > btrfs_submit_compressed_read().
> > >
> > > [CAUSE]
> > > As explicitly stated, if btrfs_alloc_page_array() returned -ENOMEM,
> > > caller is responsible to free the partially allocated pages.
> > >
> > > For the existing call sites, most of them are fine:
> > >
> > > - btrfs_raid_bio::stripe_pages
> > >   Handled by free_raid_bio().
> > >
> > > - extent_buffer::pages[]
> > >   Handled btrfs_release_extent_buffer_pages().
> > >
> > > - scrub_stripe::pages[]
> > >   Handled by release_scrub_stripe().
> > >
> > > But there is one exception in btrfs_submit_compressed_read(), if
> > > btrfs_alloc_page_array() failed, we didn't cleanup the array and freed
> > > the array pointer directly.
> > >
> > > Initially there is still the error handling in commit dd137dd1f2d7
> > > ("btrfs: factor out allocating an array of pages"), but later in commit
> > > 544fe4a903ce ("btrfs: embed a btrfs_bio into struct compressed_bio"),
> > > the error handling is removed, leading to the possible memory leak.
> > >
> > > [FIX]
> > > This patch would add back the error handling first, then to prevent such
> > > situation from happening again, also make btrfs_alloc_page_array() to
> > > free the allocated pages as a extra safe net.
> > >
> > > Fixes: 544fe4a903ce ("btrfs: embed a btrfs_bio into struct compressed_bio")
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> >
> > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> >
> > Looks good, thanks.
> 
> Well, just one comment, see below.
> 
> >
> > > ---
> > >  fs/btrfs/compression.c |  4 ++++
> > >  fs/btrfs/extent_io.c   | 10 +++++++---
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> > > index 19b22b4653c8..d6120741774b 100644
> > > --- a/fs/btrfs/compression.c
> > > +++ b/fs/btrfs/compression.c
> > > @@ -534,6 +534,10 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio)
> > >         return;
> > >
> > >  out_free_compressed_pages:
> > > +       for (int i = 0; i < cb->nr_pages; i++) {
> > > +               if (cb->compressed_pages[i])
> > > +                       __free_page(cb->compressed_pages[i]);
> > > +       }
> 
> So this hunk is not needed, because of the changes you did to
> btrfs_alloc_page_array(), as now it always frees any allocated pages
> on -ENOMEM.

Right, I'll drop the hunk, thanks.