Re: [PATCH v6 07/12] mm/cma: Introduce cma_alloc_at() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Infinite loop in `cma_range_alloc()` when the found page range is not contiguous.
- [High] Unit mismatch in `cma_range_alloc()` causes out-of-bounds bitmap accesses when `cma->order_per_bit > 0`.
- [Medium] `__cma_alloc_frozen()` dereferences the `cma` pointer before verifying that it is not NULL.
- [Medium] `cma_alloc_at()` with offset 0 bypasses multi-range protection, allowing incorrect fallback allocations.
- [Medium] `cma_alloc_at()` lacks alignment validation for `offset`, leading to silent truncation.
- [Low] The newly defined trace events `cma_alloc_at_start` and `cma_alloc_at_finish` are never emitted.
--

commit 14084cc27d73ffca7f55e0856f2f9a10ae91ca42
Author: Thierry Reding <treding@nvidia.com>
Date:   Fri Sep 4 10:44:58 2026 +0000

mm/cma: Introduce cma_alloc_at() API

This commit introduces a new cma_alloc_at() API to allocate contiguous 
memory starting at a fixed page offset. The cma_range_alloc() function 
is refactored to support range-bounded allocations, enabling the new 
API to reuse the existing allocation path.

> diff --git a/include/trace/events/cma.h b/include/trace/events/cma.h
> index 37195edf24986..4e0ab63e303f0 100644
> --- a/include/trace/events/cma.h
> +++ b/include/trace/events/cma.h
[ ... ]
> @@ -132,6 +140,69 @@ TRACE_EVENT(cma_alloc_busy_retry,
>  		  __entry->align)
>  );
>  
> +TRACE_EVENT(cma_alloc_at_start,
> +
> +	TP_PROTO(const char *name, unsigned long pfn,
> +		 unsigned long request_count, unsigned long available_count,
> +		 unsigned long total_count),

[Severity: Low]
Are these new trace events actually emitted anywhere?

It looks like cma_alloc_at() calls __cma_alloc_frozen(), which continues
to use the existing trace_cma_alloc_start() and trace_cma_alloc_finish()
events. Do we need these new definitions if they remain unused?

> diff --git a/mm/cma.c b/mm/cma.c
> index a10ea37a261da..f92d3a1882098 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
[ ... ]
>  static int cma_range_alloc(struct cma *cma, struct cma_memrange *cmr,
> -				unsigned long count, unsigned int align,
> -				struct page **pagep, gfp_t gfp)
> +			   unsigned long start, unsigned long end,
> +			   unsigned long count, unsigned int align,
> +			   struct page **pagep, gfp_t gfp)
>  {
>  	unsigned long bitmap_maxno, bitmap_no, bitmap_count;
> -	unsigned long start, pfn, mask, offset;
> -	int ret = -EBUSY;
> +	unsigned long pfn, mask, offset;
>  	struct page *page = NULL;
> +	int ret = -EBUSY;
>  
>  	mask = cma_bitmap_aligned_mask(cma, align);
>  	offset = cma_bitmap_aligned_offset(cma, cmr, align);
> -	bitmap_maxno = cma_bitmap_maxno(cma, cmr);
> +	bitmap_maxno = (end > start) ? end : cma_bitmap_maxno(cma, cmr);
>  	bitmap_count = cma_bitmap_pages_to_bits(cma, count);
>  
>  	if (bitmap_count > bitmap_maxno)
>  		goto out;
>  
> -	for (start = 0; ; start = bitmap_no + mask + 1) {
> +	while (true) {
>  		spin_lock_irq(&cma->lock);
[ ... ]
> +		bitmap_no = bitmap_find_next_zero_area_off(cmr->bitmap,
> +				bitmap_maxno, start, bitmap_count, mask,
> +				offset);

[Severity: High]
Does this code use page counts where bit indices are expected?

The end and start parameters are passed in as unscaled page counts from
cma_alloc_at(), and end is assigned directly to bitmap_maxno. If
cma->order_per_bit > 0, a single bit represents multiple pages.

Passing unscaled page counts directly as bit indices to
bitmap_find_next_zero_area_off() could cause it to read past the end of the
bitmap, and subsequently cause bitmap_set() to write out of bounds.

>  		if (bitmap_no >= bitmap_maxno) {
>  			spin_unlock_irq(&cma->lock);
>  			break;
>  		}
>  
>  		pfn = cmr->base_pfn + (bitmap_no << cma->order_per_bit);
>  		page = pfn_to_page(pfn);
>  
>  		/*
>  		 * Do not hand out page ranges that are not contiguous, so
>  		 * callers can just iterate the pages without having to worry
>  		 * about these corner cases.
>  		 */
>  		if (!page_range_contiguous(page, count)) {
>  			spin_unlock_irq(&cma->lock);
>  			pr_warn_ratelimited("%s: %s: skipping incompatible area [0x%lx-0x%lx]",
>  					    __func__, cma->name, pfn, pfn + count - 1);
>  			continue;
>  		}

[Severity: Critical]
Could this continue statement cause an infinite loop?

Since this was changed from a for loop to a while (true) loop, the
continue statement jumps directly back to the loop condition. This skips
the 'start = bitmap_no + mask + 1;' update at the end of the loop body.

The loop will retry with the exact same start index and unmodified bitmap
state, find the exact same incompatible area, and loop without terminating.

[ ... ]
> -static struct page *__cma_alloc_frozen(struct cma *cma,
> -		unsigned long count, unsigned int align, gfp_t gfp)
> +static struct page *__cma_alloc_frozen(struct cma *cma, unsigned long start,
> +				       unsigned long end, unsigned long count,
> +				       unsigned int align, gfp_t gfp)
>  {
>  	struct page *page = NULL;
>  	int ret = -ENOMEM, r;
>  	unsigned long i;
> -	const char *name = cma ? cma->name : NULL;
> +
> +	/* cma_alloc_at() and friends will only work with single-range CMA */
> +	if (WARN_ON_ONCE(start > 0 && cma->nranges != 1))
> +		return page;

[Severity: Medium]
Is there a risk of a NULL pointer dereference here?

If __cma_alloc_frozen() is called with a NULL cma pointer and a non-zero
start offset via the exported cma_alloc_at() API, cma->nranges is
evaluated before the '!cma || !cma->count' safety check below it.

[Severity: Medium]
Does this check properly guard exact-offset allocations if offset is 0?

If a caller requests an allocation exactly at offset 0, start will be 0,
which evaluates to false for the 'start > 0' condition.

This bypasses the single-range enforcement. If the first range is busy, the
code could erroneously fall back and iterate over subsequent ranges,
potentially returning an allocation at an arbitrary physical offset.

>  
>  	if (!cma || !cma->count)
>  		return page;
[ ... ]
> +/**
> + * cma_alloc_at() - allocate pages from contiguous area at fixed offset
> + * @cma:     Contiguous memory region for which the allocation is performed.
> + * @offset:  Index of the first page to allocate.
> + * @count:   Requested number of pages (in PAGE_SIZE order).
> + * @no_warn: Avoid printing message about failed allocation.
> + *
> + * This function allocates a part of the contiguous memory on a specific
> + * contiguous memory area.
> + */
> +struct page *cma_alloc_at(struct cma *cma, unsigned long offset,
> +			  unsigned long count, bool no_warn)
> +{

[Severity: Medium]
Does this API need to validate that the requested offset is aligned with
the CMA block size?

CMA allocates memory in chunks of (1 << cma->order_per_bit) pages. The
offset is passed in pages without validation. If properly scaled to bits
later, an unaligned offset would be silently truncated, allocating a block
at a rounded-down physical address instead of the exact requested offset.

> +	struct page *page;
> +
> +	page = cma_alloc_at_frozen(cma, offset, count, no_warn);
> +	if (page)
> +		set_pages_refcounted(page, count);
> +
> +	return page;
> +}
> +EXPORT_SYMBOL_GPL(cma_alloc_at);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-tegra-vpr-v6-0-79042cfa8de5@nvidia.com?part=7