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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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

Thanks,
Qu
>