Re: [RFC PATCH v8 06/14] dma-pool: Allocate CoCo atomic pools using CoCo shared memory allocator

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] dma-pool: atomic_pool_expand() falsely returns success on failure
> --
>
> commit 7de8bc3a8ea9e2bbef83e09d72fa88999ce898b9
> Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>
> dma-pool: Allocate CoCo atomic pools using CoCo shared memory allocator
>
> This commit modifies atomic DMA pools to use the CoCo shared memory allocator.
> It prevents allocation fallback below the required shared granule size, and
> uses the cc_make_shared() helper to transition the backing allocations in a
> sleepable context.
>
>> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
>> index 70b7f64b17ab5..f059312c15aa5 100644
>> --- a/kernel/dma/pool.c
>> +++ b/kernel/dma/pool.c
> [ ... ]
>> @@ -85,6 +85,9 @@ static bool cma_in_zone(gfp_t gfp)
>>  static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t pool_size,
>>  			      gfp_t gfp)
>>  {
>> +	struct cc_shared_layout layout;
>> +	unsigned long attrs = 0;
>> +	unsigned int min_order = 0;
>>  	unsigned int order;
>>  	struct page *page = NULL;
>>  	bool leak_pages = false;
>> @@ -92,6 +95,17 @@ static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t pool_size,
>>  	int ret = -ENOMEM;
>>  	pgprot_t prot __maybe_unused;
>>  
>> +	if (dma_pool->cc_shared) {
>> +		ret = cc_shared_calc_layout(pool_size, &layout);
>> +		if (ret)
>> +			goto out;
>
> [Severity: High]
> If cc_shared_calc_layout() succeeds, it returns 0 and overwrites the initial
> ret = -ENOMEM. Could this cause the function to falsely return 0 (success) on
> subsequent failures?
>
> For instance, if page allocation fails below, the function jumps to out and
> returns the current value of ret:
>
>     if (!page)
>         goto out;
>
> Similarly, if remapping fails later in the function:
>
>     addr = dma_common_contiguous_remap(...)
>     if (!addr)
>         goto free_page;
>
> In both cases, ret will be 0 instead of -ENOMEM. Would it be better to reset
> ret back to -ENOMEM when these failures occur, or use a separate variable
> for the cc_shared_calc_layout() return value?
>

Fixed as

modified    kernel/dma/pool.c
@@ -96,9 +96,11 @@ static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t pool_size,
 	pgprot_t prot __maybe_unused;
 
 	if (dma_pool->cc_shared) {
-		ret = cc_shared_calc_layout(pool_size, &layout);
-		if (ret)
-			goto out;
+		int err;
+
+		err = cc_shared_calc_layout(pool_size, &layout);
+		if (err)
+			return err;
 		pool_size = layout.shared_size;
 		min_order = get_order(layout.alignment);
 		if (min_order > MAX_PAGE_ORDER)


-aneesh