Re: [PATCH v3 06/11] mm/cma: Allow dynamically creating CMA areas

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] cma_free() causes a double free and memory corruption by incorrectly calling free_reserved_page() on pages actively managed by the buddy allocator.
- [High] Compilation fails when CONFIG_CMA_SYSFS=y or CONFIG_CMA_DEBUGFS=y because cma_areas was changed from an exported array to a static list.
- [High] cma_create() ignores cma_activate_area() failures, allowing silently broken CMA objects into the list, causing a kernel panic in cma_free().
- [High] Time-of-Check to Time-of-Use (TOCTOU) race in cma_free() allows concurrent allocations to be erroneously ripped away.
- [Medium] Incorrect teardown ordering in cma_free() allows concurrent list iterators to access a destroyed object.
- [Medium] Name collision for all unnamed early CMA areas and a lockless tracking race for dynamic areas.
- [Medium] cma_create() is strictly annotated with __init, breaking its stated purpose to allow runtime dynamic creation.
- [Low] The patch removes the decrement of totalcma_pages in the cma_activate_area() error path, permanently leaking the accounting.
--

commit cddc1d630ad599e76103540b31f303767b8a5454
Author: Thierry Reding <treding@nvidia.com>

mm/cma: Allow dynamically creating CMA areas
    
There is no technical reason why there should be a limited number of CMA
regions, so extract some code into helpers and use them to create extra
functions (cma_create() and cma_free()) that allow creating and freeing,
respectively, CMA regions dynamically at runtime.
    
The static array of CMA areas cannot be replaced by dynamically created
areas because for many of them, allocation must not fail and some cases
may need to initialize them before the slab allocator is even available.
To account for this, keep these "early" areas in a separate list and
track the dynamic areas in a separate list.

> diff --git a/mm/cma.c b/mm/cma.c
> index a13ce4999b39f..f989e2e98594f 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -34,7 +34,12 @@
>  #include "internal.h"
>  #include "cma.h"
>  
> -struct cma cma_areas[MAX_CMA_AREAS];
> +static DEFINE_MUTEX(cma_lock);
> +
> +struct cma cma_early_areas[MAX_EARLY_CMA_AREAS];
> +unsigned int cma_early_area_count;
> +
> +static LIST_HEAD(cma_areas);
>  unsigned int cma_area_count;

[Severity: High]
This patch changes cma_areas from an exported array to a static list, and
stops exposing cma_area_count. Were mm/cma_sysfs.c and mm/cma_debug.c missed
during this conversion?

For example, cma_sysfs_init() in mm/cma_sysfs.c still does:

    for (i = 0; i < cma_area_count; i++) {
        cma = &cma_areas[i];

Does this cause compilation failures when CONFIG_CMA_SYSFS=y or
CONFIG_CMA_DEBUGFS=y are enabled?

[ ... ]

> @@ -198,7 +203,6 @@ static void __init cma_activate_area(struct cma *cma)
>  				free_reserved_page(pfn_to_page(pfn));
>  		}
>  	}
> -	totalcma_pages -= cma->count;

[Severity: Low]
Is it intended to remove this decrement? If an early area fails to activate,
it appears the global totalcma_pages variable will remain artificially
inflated forever, corrupting CmaTotal statistics.

>  	cma->available_count = cma->count = 0;
>  	pr_err("CMA area %s could not be activated\n", cma->name);
>  }

[ ... ]

> @@ -219,41 +223,77 @@ void __init cma_reserve_pages_on_error(struct cma *cma)
>  	set_bit(CMA_RESERVE_PAGES_ON_ERROR, &cma->flags);
>  }
>  
> +static void __init cma_init_area(struct cma *cma, const char *name,
> +				 phys_addr_t size, unsigned int order_per_bit)
> +{
> +	if (name)
> +		strscpy(cma->name, name);
> +	else
> +		snprintf(cma->name, CMA_MAX_NAME,  "cma%d\n", cma_area_count);

[Severity: Medium]
Will this use the correct counter for early areas? Since early areas now
correctly increment cma_early_area_count, cma_area_count will remain strictly
at 0 throughout early boot. Will all unnamed early areas be assigned the
exact same name ('cma0\n'), potentially causing kobject registration
collisions?

Also, when cma_create() calls this locklessly, can concurrent dynamic creations
read the same cma_area_count and generate duplicate names?

> +
> +	cma->available_count = cma->count = size >> PAGE_SHIFT;
> +	cma->order_per_bit = order_per_bit;
> +
> +	INIT_LIST_HEAD(&cma->node);
> +}

[ ... ]

> @@ -1147,3 +1193,74 @@ void __init *cma_reserve_early(struct cma *cma, unsigned long size)
>  
>  	return ret;
>  }
> +
> +struct cma *__init cma_create(phys_addr_t base, phys_addr_t size,
> +			      unsigned int order_per_bit, const char *name)

[Severity: Medium]
The commit message states the goal is to allow creating regions dynamically at
runtime. Does the __init annotation here and on cma_check_memory() prevent
that? 

If a subsystem attempts to use this API dynamically at true runtime, won't it
branch into freed memory since the kernel permanently discards .init.text
after early boot?

> +{
> +	struct cma *cma;
> +	int ret;
> +
> +	ret = cma_check_memory(base, size);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	cma = kzalloc_obj(*cma, GFP_KERNEL);
> +	if (!cma)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cma_init_area(cma, name, size, order_per_bit);
> +	cma->ranges[0].base_pfn = PFN_DOWN(base);
> +	cma->ranges[0].early_pfn = PFN_DOWN(base);
> +	cma->ranges[0].count = cma->count;
> +	cma->nranges = 1;
> +
> +	cma_activate_area(cma);
> +
> +	mutex_lock(&cma_lock);
> +	list_add_tail(&cma->node, &cma_areas);

[Severity: High]
cma_activate_area() returns void, so any failures inside it cannot be checked
here. If activation fails, cma_activate_area() frees the partial bitmaps and
zeroes cma->count, but leaves cmr->count intact. 

Does adding this potentially broken object to the list cause a kernel panic
later if cma_free() is called on it? cma_free() would calculate nbits from
the unmodified cmr->count and call bitmap_empty() with a freed bitmap pointer.

> +	totalcma_pages += cma->count;
> +	cma_area_count++;
> +	mutex_unlock(&cma_lock);
> +
> +	return cma;
> +}
> +
> +void cma_free(struct cma *cma)
> +{
> +	unsigned int i;
> +
> +	/*
> +	 * Safety check to prevent a CMA with active allocations from being
> +	 * released.
> +	 */
> +	for (i = 0; i < cma->nranges; i++) {
> +		unsigned long nbits = cma_bitmap_maxno(cma, &cma->ranges[i]);
> +
> +		if (!bitmap_empty(cma->ranges[i].bitmap, nbits)) {
> +			WARN(1, "%s: range %u not empty\n", cma->name, i);
> +			return;
> +		}

[Severity: High]
Does this lockless emptiness check suffer from a Time-of-Check to Time-of-Use
(TOCTOU) race? 

Because the check doesn't hold cma->alloc_mutex or cma->lock, couldn't a
concurrent cma_alloc() successfully acquire the mutex and allocate pages from
this region immediately after the lockless emptiness check succeeds?

> +	}
> +
> +	/* free reserved pages and the bitmap */
> +	for (i = 0; i < cma->nranges; i++) {
> +		struct cma_memrange *cmr = &cma->ranges[i];
> +		unsigned long end_pfn, pfn;
> +
> +		end_pfn = cmr->base_pfn + cmr->count;
> +		for (pfn = cmr->base_pfn; pfn < end_pfn; pfn++)
> +			free_reserved_page(pfn_to_page(pfn));

[Severity: Critical]
Will this loop cause a double free and memory corruption?

Successfully activated CMA pages are unreserved via
init_cma_reserved_pageblock(), which clears PG_reserved and hands them to the
buddy allocator. If free_reserved_page() is called on pages that are actively
managed by the buddy allocator or in use by callers, won't it reset the page
refcount and explicitly call __free_page() again, corrupting the buddy
allocator's free lists?

> +
> +		bitmap_free(cmr->bitmap);
> +	}
> +
> +	mutex_destroy(&cma->alloc_mutex);
> +
> +	mutex_lock(&cma_lock);
> +	totalcma_pages -= cma->count;
> +	list_del(&cma->node);

[Severity: Medium]
Is it safe to destroy cma->alloc_mutex and free the bitmaps before unlinking
the object from the global cma_areas list?

If a concurrent thread is executing cma_for_each_area(), it traverses the list
under cma_lock and might obtain a pointer to a struct cma that is midway
through teardown, potentially accessing a destroyed mutex or freed bitmaps.
Should the unlinking happen before the resources are destroyed?

> +	cma_area_count--;
> +	mutex_unlock(&cma_lock);
> +
> +	kfree(cma);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-tegra-vpr-v3-0-d80f7b871bb4@nvidia.com?part=6