[email protected] 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) <[email protected]>
>
> 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
