[PATCH] cma: fail if fixed declaration can't be honored

[PATCH] cma: fail if fixed declaration can't be honored

[Doug Berger]

The description of the cma_declare_contiguous() function indicates
that if the 'fixed' argument is true the reserved contiguous area
must be exactly at the address of the 'base' argument.

However, the function currently allows the 'base', 'size', and
'limit' arguments to be silently adjusted to meet alignment
constraints. This commit enforces the documented behavior through
explicit checks that return an error if the region does not fit
within a specified region.

Fixes: 5ea3b1b2f8ad ("cma: add placement specifier for "cma=" kernel parameter")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 mm/cma.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/mm/cma.c b/mm/cma.c
index 3340ef34c154..4973d253dc83 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -278,6 +278,12 @@ int __init cma_declare_contiguous(phys_addr_t base,
 	 */
 	alignment = max(alignment,  (phys_addr_t)PAGE_SIZE <<
 			  max_t(unsigned long, MAX_ORDER - 1, pageblock_order));
+	if (fixed && base & (alignment - 1)) {
+		ret = -EINVAL;
+		pr_err("Region at %pa must be aligned to %pa bytes\n",
+			&base, &alignment);
+		goto err;
+	}
 	base = ALIGN(base, alignment);
 	size = ALIGN(size, alignment);
 	limit &= ~(alignment - 1);
@@ -308,6 +314,13 @@ int __init cma_declare_contiguous(phys_addr_t base,
 	if (limit == 0 || limit > memblock_end)
 		limit = memblock_end;
 
+	if (base + size > limit) {
+		ret = -EINVAL;
+		pr_err("Size (%pa) of region at %pa exceeds limit (%pa)\n",
+			&size, &base, &limit);
+		goto err;
+	}
+
 	/* Reserve memory */
 	if (fixed) {
 		if (memblock_is_region_reserved(base, size) ||
-- 
2.7.4

Re: [PATCH] cma: fail if fixed declaration can't be honored

[Michał Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

On Tue, 25 Jun 2019 at 01:22, Doug Berger <opendmb@gmail.com> wrote:

> The description of the cma_declare_contiguous() function indicates
> that if the 'fixed' argument is true the reserved contiguous area
> must be exactly at the address of the 'base' argument.
>
> However, the function currently allows the 'base', 'size', and
> 'limit' arguments to be silently adjusted to meet alignment
> constraints. This commit enforces the documented behavior through
> explicit checks that return an error if the region does not fit
> within a specified region.
>
> Fixes: 5ea3b1b2f8ad ("cma: add placement specifier for "cma=" kernel
> parameter")
> Signed-off-by: Doug Berger <opendmb@gmail.com>
>

Acked-by: Michal Nazarewicz <mina86@mina86.com>


> ---
>  mm/cma.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/mm/cma.c b/mm/cma.c
> index 3340ef34c154..4973d253dc83 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -278,6 +278,12 @@ int __init cma_declare_contiguous(phys_addr_t base,
>          */
>         alignment = max(alignment,  (phys_addr_t)PAGE_SIZE <<
>                           max_t(unsigned long, MAX_ORDER - 1,
> pageblock_order));
> +       if (fixed && base & (alignment - 1)) {
> +               ret = -EINVAL;
> +               pr_err("Region at %pa must be aligned to %pa bytes\n",
> +                       &base, &alignment);
> +               goto err;
> +       }
>
        base = ALIGN(base, alignment);
>         size = ALIGN(size, alignment);
>         limit &= ~(alignment - 1);
> @@ -308,6 +314,13 @@ int __init cma_declare_contiguous(phys_addr_t base,
>         if (limit == 0 || limit > memblock_end)
>                 limit = memblock_end;
>
> +       if (base + size > limit) {
> +               ret = -EINVAL;
> +               pr_err("Size (%pa) of region at %pa exceeds limit (%pa)\n",
> +                       &size, &base, &limit);
> +               goto err;
> +       }
> +
>         /* Reserve memory */
>         if (fixed) {
>                 if (memblock_is_region_reserved(base, size) ||
> --
> 2.7.4
>
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[-- Attachment #2: Type: text/html, Size: 3398 bytes --]

Re: [PATCH] cma: fail if fixed declaration can't be honored

[Andrew Morton]

On Mon, 24 Jun 2019 17:20:51 -0700 Doug Berger <opendmb@gmail.com> wrote:

> The description of the cma_declare_contiguous() function indicates
> that if the 'fixed' argument is true the reserved contiguous area
> must be exactly at the address of the 'base' argument.
> 
> However, the function currently allows the 'base', 'size', and
> 'limit' arguments to be silently adjusted to meet alignment
> constraints. This commit enforces the documented behavior through
> explicit checks that return an error if the region does not fit
> within a specified region.
> 
> ...
>
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -278,6 +278,12 @@ int __init cma_declare_contiguous(phys_addr_t base,
>  	 */
>  	alignment = max(alignment,  (phys_addr_t)PAGE_SIZE <<
>  			  max_t(unsigned long, MAX_ORDER - 1, pageblock_order));
> +	if (fixed && base & (alignment - 1)) {
> +		ret = -EINVAL;
> +		pr_err("Region at %pa must be aligned to %pa bytes\n",
> +			&base, &alignment);

CMA functions do like to use pr_err() when the caller messed something
up.  It should be using WARN_ON() or WARN_ON_ONCE(), mainly so we get a
backtrace to find out which caller messed up.

There are probably other sites which should be converted, but I think
it would be best to get these new ones correct.  So something like

	if (WARN_ONCE(fixed && base & (alignment - 1)),
		      "region at %pa must be aligned to %pa bytes",
		      &base, &alignment) {
		ret = -EINVAL;
		goto err;
	}