Re: [PATCH v4 05/10] mm/cma: Allow dynamically creating CMA areas

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] cma_free() double-frees pages into the buddy allocator, causing memory corruption.
- [High] cma_create() ignores the return value of cma_activate_area(), publishing broken and partially-initialized CMA areas.
- [High] cma_create() relies on memblock_is_region_reserved() at runtime, which is discarded after early boot.
- [High] cma_for_each_area() only iterates over dynamic areas, breaking APIs relying on scanning all CMA areas.
- [High] cma_free() checks if the CMA area is empty without holding the allocation mutex, leading to a TOCTOU race with concurrent allocations.
- [Medium] totalcma_pages is permanently inflated if an early CMA area fails to activate.
- [Medium] cma_create() fails to initialize cma->nid, defaulting it to 0 and potentially breaking zone validation on NUMA systems.
- [Low] Dynamically created CMA areas are missing from sysfs and debugfs interfaces.
--

--- Patch [5]: [PATCH v4 05/10] mm/cma: Allow dynamically creating CMA areas ---
commit ef1094abdd1bc24a286e7eaebd2724d471aebbfd
Author: Thierry Reding <[email protected]>

mm/cma: Allow dynamically creating CMA areas

This patch introduces cma_create() and cma_free() to allow dynamically 
creating and freeing CMA regions at runtime. It separates static early areas 
from dynamically allocated ones into different tracking structures.

> diff --git a/mm/cma.c b/mm/cma.c
> index a7929c758df15..8d8fe7c82ae5b 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
[ ... ]
> @@ -198,19 +207,24 @@ static void __init cma_activate_area(struct cma *cma)
>  			end_pfn = cmr->base_pfn + cmr->count;
>  			for (pfn = start_pfn; pfn < end_pfn; pfn++)
>  				free_reserved_page(pfn_to_page(pfn));
> +
> +			/* reset these so we don't later confuse cma_free() */
> +			cmr->bitmap = NULL;
> +			cmr->count = 0;
>  		}
>  	}
> -	totalcma_pages -= cma->count;
>  	cma->available_count = cma->count = 0;
>  	pr_err("CMA area %s could not be activated\n", cma->name);
> +
> +	return err;
>  }

[Severity: Medium]
Does this removal leave totalcma_pages permanently inflated if an early boot
area fails to activate? Early areas still have their page count added during
cma_new_area() prior to activation.

[ ... ]
> @@ -221,41 +235,85 @@ void __init cma_reserve_pages_on_error(struct cma *cma)
[ ... ]
> +static int cma_check_memory(phys_addr_t base, phys_addr_t size)
> +{
> +	if (!size || !memblock_is_region_reserved(base, size))
> +		return -EINVAL;

[Severity: High]
Is it safe to call memblock_is_region_reserved() from cma_create() at runtime?
Since memblock_is_region_reserved() and memblock data structures are marked
__init_memblock, won't they be discarded after early boot on architectures
without CONFIG_ARCH_KEEP_MEMBLOCK, leading to a use-after-free?

[ ... ]
> @@ -1053,12 +1098,12 @@ bool cma_release_frozen(struct cma *cma, const struct page *pages,
[ ... ]
> +int cma_for_each_area(int (*it)(struct cma *cma, void *data), void *data)
> +{
> +	struct cma *cma;
> +
> +	mutex_lock(&cma_lock);
> +
> +	list_for_each_entry(cma, &cma_areas, node) {

[Severity: High]
Since cma_areas only contains dynamically created regions, will callers of
cma_for_each_area() now silently skip all standard early-boot CMA areas?

[ ... ]
> @@ -1149,3 +1213,78 @@ void __init *cma_reserve_early(struct cma *cma, unsigned long size)
[ ... ]
> +struct cma *cma_create(phys_addr_t base, phys_addr_t size,
> +		       unsigned int order_per_bit, const char *name)
> +{
> +	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);

[Severity: Medium]
Should cma->nid be initialized to NUMA_NO_NODE here? Since kzalloc_obj()
leaves it as 0, could this cause pfn_range_intersects_zones() to validate
against Node 0 instead of the correct node, potentially bypassing NUMA zone
overlap checks?

> +
> +	mutex_lock(&cma_lock);
> +
> +	cma_init_area(cma, name, size, order_per_bit, true);
> +	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);
> +
> +	list_add_tail(&cma->node, &cma_areas);

[Severity: High]
What happens if cma_activate_area() fails (e.g., due to an -ENOMEM during
bitmap allocation)? The error code is discarded, and the partially-initialized
cma object (with uninitialized mutexes and locks) is added to the global
cma_areas list. Could this cause crashes when the caller or other subsystems
try to use or free it?

> +	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 (nbits && !bitmap_empty(cma->ranges[i].bitmap, nbits)) {
> +			WARN(1, "%s: range %u not empty\n", cma->name, i);
> +			return;
> +		}
> +	}

[Severity: High]
Can a concurrent thread successfully allocate from this CMA area right after
the bitmap_empty() check completes? The check is performed outside of
cma->alloc_mutex, so a race could lead to destroying the mutex and freeing
the underlying memory while it is actively being allocated.

> +
> +	mutex_lock(&cma_lock);
> +
> +	/* 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]
Does this loop double-free pages into the buddy allocator? When
cma_activate_area() is initially called, it invokes
init_cma_reserved_pageblock() which clears the PageReserved flag and inserts
the memory into the buddy allocator. If cma_free() later iterates over these
pages and calls free_reserved_page(), won't that blindly call __free_pages()
on pages already managed by the buddy allocator, corrupting the free lists?

[ ... ]
> diff --git a/mm/cma_debug.c b/mm/cma_debug.c
> index 523ba4a0f9f76..548b0157e7b5a 100644
> --- a/mm/cma_debug.c
> +++ b/mm/cma_debug.c
> @@ -204,9 +204,9 @@ static int __init cma_debugfs_init(void)
>  
>  	cma_debugfs_root = debugfs_create_dir("cma", NULL);
>  
> -	for (i = 0; i < cma_area_count; i++)
> -		if (test_bit(CMA_ACTIVATED, &cma_areas[i].flags))
> -			cma_debugfs_add_one(&cma_areas[i], cma_debugfs_root);
> +	for (i = 0; i < cma_early_area_count; i++)
> +		if (test_bit(CMA_ACTIVATED, &cma_early_areas[i].flags))
> +			cma_debugfs_add_one(&cma_early_areas[i], cma_debugfs_root);

[Severity: Low]
Since sysfs and debugfs initialization now strictly iterate over
cma_early_areas during boot, are dynamically created CMA areas intentionally
excluded from these interfaces?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5