| Message ID | 0f34dd9fbefc379a65fe09074f975a199352d99e.1700796515.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: free the allocated memory if btrfs_alloc_page_array() failed | expand |
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. > --- > 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]); > + } > kfree(cb->compressed_pages); > out_free_bio: > bio_put(&cb->bbio.bio); > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 0ea65f248c15..e2c0c596bd46 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -676,8 +676,7 @@ static void end_bio_extent_readpage(struct btrfs_bio *bbio) > * the array will be skipped > * > * Return: 0 if all pages were able to be allocated; > - * -ENOMEM otherwise, and the caller is responsible for freeing all > - * non-null page pointers in the array. > + * -ENOMEM otherwise, and the partially allocated pages would be freed. > */ > int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) > { > @@ -696,8 +695,13 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) > * though alloc_pages_bulk_array() falls back to alloc_page() > * if it could not bulk-allocate. So we must be out of memory. > */ > - if (allocated == last) > + if (allocated == last) { > + for (int i = 0; i < allocated; i++) { > + __free_page(page_array[i]); > + page_array[i] = NULL; > + } > return -ENOMEM; > + } > > memalloc_retry_wait(GFP_NOFS); > } > -- > 2.42.1 > >
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. Thanks. > > kfree(cb->compressed_pages); > > out_free_bio: > > bio_put(&cb->bbio.bio); > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > index 0ea65f248c15..e2c0c596bd46 100644 > > --- a/fs/btrfs/extent_io.c > > +++ b/fs/btrfs/extent_io.c > > @@ -676,8 +676,7 @@ static void end_bio_extent_readpage(struct btrfs_bio *bbio) > > * the array will be skipped > > * > > * Return: 0 if all pages were able to be allocated; > > - * -ENOMEM otherwise, and the caller is responsible for freeing all > > - * non-null page pointers in the array. > > + * -ENOMEM otherwise, and the partially allocated pages would be freed. > > */ > > int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) > > { > > @@ -696,8 +695,13 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) > > * though alloc_pages_bulk_array() falls back to alloc_page() > > * if it could not bulk-allocate. So we must be out of memory. > > */ > > - if (allocated == last) > > + if (allocated == last) { > > + for (int i = 0; i < allocated; i++) { > > + __free_page(page_array[i]); > > + page_array[i] = NULL; > > + } > > return -ENOMEM; > > + } > > > > memalloc_retry_wait(GFP_NOFS); > > } > > -- > > 2.42.1 > > > >
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.
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 >
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.
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]); + } kfree(cb->compressed_pages); out_free_bio: bio_put(&cb->bbio.bio); diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 0ea65f248c15..e2c0c596bd46 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -676,8 +676,7 @@ static void end_bio_extent_readpage(struct btrfs_bio *bbio) * the array will be skipped * * Return: 0 if all pages were able to be allocated; - * -ENOMEM otherwise, and the caller is responsible for freeing all - * non-null page pointers in the array. + * -ENOMEM otherwise, and the partially allocated pages would be freed. */ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) { @@ -696,8 +695,13 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) * though alloc_pages_bulk_array() falls back to alloc_page() * if it could not bulk-allocate. So we must be out of memory. */ - if (allocated == last) + if (allocated == last) { + for (int i = 0; i < allocated; i++) { + __free_page(page_array[i]); + page_array[i] = NULL; + } return -ENOMEM; + } memalloc_retry_wait(GFP_NOFS); }
[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> --- fs/btrfs/compression.c | 4 ++++ fs/btrfs/extent_io.c | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)