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

[Aneesh Kumar K . V]

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.


>
>>  	}
>>  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