Re: [PATCH] dma-contiguous: support percentage sizes in cma_pernuma= and numa_cma=

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] dma-contiguous: numa_cma_parse() accepts empty string and
  trailing comma
- [Medium] dma-contiguous: zero byte size fails to override percentage
  in dma_numa_cma_reserve()
--

commit 3226f812115ddc4c4b57efe08a3f196e1ea740b7
Author: Alexander Graf <graf@amazon.com>

dma-contiguous: support percentage sizes in cma_pernuma= and numa_cma=

This commit extends the cma_pernuma= and numa_cma= boot parameters to
accept percentage-based sizes with an optional byte amount to subtract,
and validates the parameter string before applying it.

> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index 90937dc7c627d..c04d50ee0f7c8 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -135,45 +136,132 @@ EXPORT_SYMBOL_GPL(dev_get_cma_area);
>  static struct cma *dma_contiguous_numa_area[MAX_NUMNODES];
>  static phys_addr_t numa_cma_size[MAX_NUMNODES] __initdata;
>  static phys_addr_t pernuma_size_bytes __initdata;
> +static unsigned int numa_cma_pct[MAX_NUMNODES] __initdata;
> +static unsigned int pernuma_pct __initdata;
> +static phys_addr_t numa_cma_sub[MAX_NUMNODES] __initdata;
> +static phys_addr_t pernuma_sub __initdata;
>  static bool numa_cma_configured __initdata;
>  
> -static int __init early_numa_cma(char *p)
> +/*
> + * What numa_cma_parse() does with a string it accepts: NUMA_CMA_CHECK leaves
> + * everything outside the function as it is, so a caller can validate the
> + * whole string before NUMA_CMA_APPLY lets any part of it take effect.
> + */
> +enum numa_cma_action {
> +	NUMA_CMA_CHECK,
> +	NUMA_CMA_APPLY,
> +};
> +
> +/*
> + * Parse one numa_cma= string: a comma separated list of <node>:<size>, where
> + * <size> is a size or <percent>%[-<size>]. Anything else fails the parse.
> + */
> +static int __init numa_cma_parse(char *p, enum numa_cma_action action)
>  {
>  	int nid, count = 0;
>  	unsigned long node;
> -	phys_addr_t size;
> -	char *s = p;
> +	phys_addr_t size, sub;
> +	char *s = p, *start;
>  
>  	while (*s) {
> -		if (sscanf(s, "%lu%n", &node, &count) != 1)
> -			break;
> -
> -		if (s[count] == ':') {
> -			if (node >= MAX_NUMNODES)
> -				break;
> -			nid = array_index_nospec(node, MAX_NUMNODES);
> -
> -			s += count + 1;
> -			size = memparse(s, &s);
> +		if (sscanf(s, "%lu%n", &node, &count) != 1 || s[count] != ':' ||
> +		    node >= MAX_NUMNODES)
> +			return -EINVAL;
> +		nid = array_index_nospec(node, MAX_NUMNODES);
> +
> +		start = s + count + 1;
> +		size = memparse(start, &s);
> +		if (s == start)
> +			return -EINVAL;
> +
> +		if (*s == '%') {
> +			s++;
> +			if (size > 100) {
> +				pr_warn("numa_cma: node %d percentage %llu%% > 100\n",
> +					nid, (unsigned long long)size);
> +				return -EINVAL;
> +			}
> +			sub = 0;
> +			if (*s == '-') {
> +				start = ++s;
> +				sub = memparse(start, &s);
> +				if (s == start)
> +					return -EINVAL;
> +			}
> +			if (action == NUMA_CMA_APPLY) {
> +				numa_cma_pct[nid] = size;
> +				numa_cma_sub[nid] = sub;
> +			}
> +		} else if (action == NUMA_CMA_APPLY) {
>  			numa_cma_size[nid] = size;
> +		}
>  
> -			if (*s == ',')
> -				s++;
> -			else
> -				break;
> -		} else
> -			break;
> +		if (*s == ',')
> +			s++;
> +		else if (*s)
> +			return -EINVAL;
>  	}
>  
> -	numa_cma_configured = true;
>  	return 0;
>  }

[Severity: Medium]
Does numa_cma_parse() accept an empty string or a trailing comma?

When s is empty initially (numa_cma=) or after advancing past a trailing
comma (such as numa_cma=0:4G,), while (*s) terminates and returns 0
instead of -EINVAL.  On a trailing comma, the NUMA_CMA_CHECK pass
succeeds and NUMA_CMA_APPLY applies the preceding entries rather than
rejecting the malformed parameter as documented.

[ ... ]

> @@ -214,16 +329,37 @@ static void __init dma_numa_cma_reserve(void)
>  		int ret;
>  
>  		if (!node_online(nid)) {
> -			if (pernuma_size_bytes || numa_cma_size[nid])
> +			if (pernuma_size_bytes || pernuma_pct ||
> +			    numa_cma_size[nid] || numa_cma_pct[nid])
>  				pr_warn("invalid node %d specified\n", nid);
>  			continue;
>  		}
>  
>  		/* per-node numa setting has the priority */
> -		size = numa_cma_size[nid] ?: pernuma_size_bytes;
> -		if (!size)
> +		if (numa_cma_size[nid])
> +			size = numa_cma_size[nid];
> +		else if (numa_cma_pct[nid])
> +			size = numa_cma_pct_size(nid, numa_cma_pct[nid],
> +						 numa_cma_sub[nid]);
> +		else if (pernuma_size_bytes)
> +			size = pernuma_size_bytes;
> +		else if (pernuma_pct)
> +			size = numa_cma_pct_size(nid, pernuma_pct, pernuma_sub);
> +		else
>  			continue;

[Severity: Medium]
What happens when an explicit byte size of 0 is given alongside a
percentage, such as appending cma_pernuma=0 to override an earlier
cma_pernuma=80%, or passing numa_cma=0:50%,0:0?

Parsing an absolute size leaves pernuma_pct or numa_cma_pct[nid] intact,
and dma_numa_cma_reserve() treats pernuma_size_bytes == 0 and
numa_cma_size[nid] == 0 as unset.  Instead of the absolute 0 taking
priority and disabling the reservation as documented in
kernel-parameters.txt, it falls through to the percentage and still
reserves the CMA area.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111140440.43024-1-graf@amazon.com?part=1