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

[David Sterba <dsterba@suse.cz>]

On Sat, Nov 25, 2023 at 07:16:07PM +1030, Qu Wenruo wrote:
> On 2023/11/25 00:55, David Sterba wrote:
> > 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.
> 
> In that case you may also want to delete the following commit message:
> 
>  > <<This patch would add back the error handling first, then>> to
> prevent such

Updated, thanks.