On Tue, Jul 21, 2026 at 08:11:34PM +0530, Aneesh Kumar K.V wrote: > Leon Romanovsky <[email protected]> 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 <[email protected]> > >> Tested-by: Michael Kelley <[email protected]> > >> Tested-by: Mostafa Saleh <[email protected]> > >> Signed-off-by: Aneesh Kumar K.V (Arm) <[email protected]> > >> --- > >> 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 >
