Re: [PATCH v8 02/23] dma-pool: fix page leak in atomic_pool_expand() cleanup

[Leon Romanovsky]

On Tue, Jul 21, 2026 at 08:11:34PM +0530, Aneesh Kumar K.V wrote:
> Leon Romanovsky <leon@xxxxxxxxxx> writes:
> 
> > On Fri, Jul 17, 2026 at 11:34:20PM +0530, Aneesh Kumar K.V (Arm) wrote:
> >> atomic_pool_expand() frees the allocated pages from the remove_mapping
> >> error path only when CONFIG_DMA_DIRECT_REMAP is enabled.
> >> 
> >> When CONFIG_DMA_DIRECT_REMAP is disabled, failures after page allocation,
> >> such as gen_pool_add_virt(), jump to remove_mapping and return without
> >> freeing the pages.
> >> 
> >> Move __free_pages(page, order) out of the CONFIG_DMA_DIRECT_REMAP block so
> >> that cleanup paths always release the allocation.
> >> 
> >> Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> >> Tested-by: Michael Kelley <mhklinux@xxxxxxxxxxx>
> >> Tested-by: Mostafa Saleh <smostafa@xxxxxxxxxx>
> >> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@xxxxxxxxxx>
> >> ---
> >>  kernel/dma/pool.c | 10 +++++++---
> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> >> index 2b2fbb709242..b0303efbc153 100644
> >> --- a/kernel/dma/pool.c
> >> +++ b/kernel/dma/pool.c
> >> @@ -81,6 +81,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> >>  {
> >>  	unsigned int order;
> >>  	struct page *page = NULL;
> >> +	bool leak_pages = false;
> >>  	void *addr;
> >>  	int ret = -ENOMEM;
> >>  
> >> @@ -115,8 +116,10 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> >>  	 */
> >>  	ret = set_memory_decrypted((unsigned long)page_to_virt(page),
> >>  				   1 << order);
> >> -	if (ret)
> >> +	if (ret) {
> >> +		leak_pages = true;
> >>  		goto remove_mapping;
> >> +	}
> >>  	ret = gen_pool_add_virt(pool, (unsigned long)addr, page_to_phys(page),
> >>  				pool_size, NUMA_NO_NODE);
> >>  	if (ret)
> >> @@ -130,14 +133,15 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> >>  				   1 << order);
> >>  	if (WARN_ON_ONCE(ret)) {
> >>  		/* Decrypt succeeded but encrypt failed, purposely leak */
> >> -		goto out;
> >> +		leak_pages = true;
> >
> > Instead of doing this dance with temporal variable, change "goto out" to
> > be "return true".
> >
> 
> 
> I didn't follow the return true part. A failure in
> set_memory_encrypted() or set_memory_decrypted() requires the pages to
> be leaked, so this is not the only call site that sets leak_pages =
> true. There is a similar case a few lines above.

Comment about "leaked" is enough. There is no need to introduce
convoluted flow just to check that page != NULL.

Thanks

> 
> 
> >
> >>  	}
> >>  remove_mapping:
> >>  #ifdef CONFIG_DMA_DIRECT_REMAP
> >>  	dma_common_free_remap(addr, pool_size);
> >>  free_page:
> >
> > Remove free_page label, and change leftover of "goto free_page" to be
> > "goto out"
> >
> >> -	__free_pages(page, order);
> >>  #endif
> >> +	if (!leak_pages)
> >> +		__free_pages(page, order);
> >
> > Put these checks under out label and rely on page != NULL as a marker.
> > if (page)
> >  __free_pages(page, order);
> >
> >>  out:
> >>  	return ret;
> >>  }
> >> -- 
> >> 2.43.0
> >> 
> >> 
> 
> -aneesh
>