[PATCH] of: Check for overlap in reserved memory regions

[PATCH] of: Check for overlap in reserved memory regions

[Mitchel Humpherys]

Any overlap in the reserved memory regions (those specified in the
reserved-memory DT node) is a bug.  These bugs might go undetected as
long as the contested region isn't used simultaneously by multiple
software agents, which makes such bugs hard to debug.  Fix this by
printing a scary warning during boot if overlap is detected.

Signed-off-by: Mitchel Humpherys <mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/of/of_reserved_mem.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 726ebe792813..5246d346cee0 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -1,7 +1,7 @@
 /*
  * Device tree based initialization code for reserved memory.
  *
- * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
+ * Copyright (c) 2013, 2015 The Linux Foundation. All Rights Reserved.
  * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
  *		http://www.samsung.com
  * Author: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@@ -20,9 +20,11 @@
 #include <linux/mm.h>
 #include <linux/sizes.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/sort.h>
 
 #define MAX_RESERVED_REGIONS	16
 static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
+static struct reserved_mem sorted_reserved_mem[MAX_RESERVED_REGIONS] __initdata;
 static int reserved_mem_count;
 
 #if defined(CONFIG_HAVE_MEMBLOCK)
@@ -197,12 +199,53 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
 	return -ENOENT;
 }
 
+static int __init __rmem_cmp(const void *a, const void *b)
+{
+	const struct reserved_mem *ra = a, *rb = b;
+
+	return ra->base - rb->base;
+}
+
+static void __init __rmem_check_for_overlap(void)
+{
+	int i;
+
+	if (reserved_mem_count < 2)
+		return;
+
+	memcpy(sorted_reserved_mem, reserved_mem, sizeof(sorted_reserved_mem));
+	sort(sorted_reserved_mem, reserved_mem_count,
+	     sizeof(sorted_reserved_mem[0]), __rmem_cmp, NULL);
+	for (i = 0; i < reserved_mem_count - 1; i++) {
+		struct reserved_mem *this, *next;
+
+		this = &sorted_reserved_mem[i];
+		next = &sorted_reserved_mem[i + 1];
+		if (!(this->base && next->base))
+			continue;
+		if (this->base + this->size > next->base) {
+			phys_addr_t this_end, next_end;
+
+			this_end = this->base + this->size;
+			next_end = next->base + next->size;
+			WARN(1, "Reserved mem: OVERLAP DETECTED!\n");
+			pr_err("%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
+			       this->name, &this->base, &this_end,
+			       next->name, &next->base, &next_end);
+		}
+	}
+}
+
 /**
  * fdt_init_reserved_mem - allocate and init all saved reserved memory regions
  */
 void __init fdt_init_reserved_mem(void)
 {
 	int i;
+
+	/* check for overlapping reserved regions */
+	__rmem_check_for_overlap();
+
 	for (i = 0; i < reserved_mem_count; i++) {
 		struct reserved_mem *rmem = &reserved_mem[i];
 		unsigned long node = rmem->fdt_node;
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: Check for overlap in reserved memory regions

[Rob Herring]

On Fri, Sep 11, 2015 at 12:31 PM, Mitchel Humpherys
<mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug.  These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug.  Fix this by
> printing a scary warning during boot if overlap is detected.
>
> Signed-off-by: Mitchel Humpherys <mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  drivers/of/of_reserved_mem.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..5246d346cee0 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -1,7 +1,7 @@
>  /*
>   * Device tree based initialization code for reserved memory.
>   *
> - * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
> + * Copyright (c) 2013, 2015 The Linux Foundation. All Rights Reserved.
>   * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
>   *             http://www.samsung.com
>   * Author: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> @@ -20,9 +20,11 @@
>  #include <linux/mm.h>
>  #include <linux/sizes.h>
>  #include <linux/of_reserved_mem.h>
> +#include <linux/sort.h>
>
>  #define MAX_RESERVED_REGIONS   16
>  static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
> +static struct reserved_mem sorted_reserved_mem[MAX_RESERVED_REGIONS] __initdata;
>  static int reserved_mem_count;
>
>  #if defined(CONFIG_HAVE_MEMBLOCK)
> @@ -197,12 +199,53 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>         return -ENOENT;
>  }
>
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> +       const struct reserved_mem *ra = a, *rb = b;
> +
> +       return ra->base - rb->base;
> +}
> +
> +static void __init __rmem_check_for_overlap(void)
> +{
> +       int i;
> +
> +       if (reserved_mem_count < 2)
> +               return;
> +
> +       memcpy(sorted_reserved_mem, reserved_mem, sizeof(sorted_reserved_mem));
> +       sort(sorted_reserved_mem, reserved_mem_count,
> +            sizeof(sorted_reserved_mem[0]), __rmem_cmp, NULL);

Why not just sort reserved_mem?

> +       for (i = 0; i < reserved_mem_count - 1; i++) {
> +               struct reserved_mem *this, *next;
> +
> +               this = &sorted_reserved_mem[i];
> +               next = &sorted_reserved_mem[i + 1];
> +               if (!(this->base && next->base))
> +                       continue;
> +               if (this->base + this->size > next->base) {
> +                       phys_addr_t this_end, next_end;
> +
> +                       this_end = this->base + this->size;
> +                       next_end = next->base + next->size;
> +                       WARN(1, "Reserved mem: OVERLAP DETECTED!\n");
> +                       pr_err("%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",

This seems overly verbose having both WARN and pr_err. I'd combine
these. I don't think the stack trace from a WARN is too useful here
given it is the DT file that users will need to go look at.

> +                              this->name, &this->base, &this_end,
> +                              next->name, &next->base, &next_end);
> +               }
> +       }
> +}
> +
>  /**
>   * fdt_init_reserved_mem - allocate and init all saved reserved memory regions
>   */
>  void __init fdt_init_reserved_mem(void)
>  {
>         int i;
> +
> +       /* check for overlapping reserved regions */
> +       __rmem_check_for_overlap();
> +
>         for (i = 0; i < reserved_mem_count; i++) {
>                 struct reserved_mem *rmem = &reserved_mem[i];
>                 unsigned long node = rmem->fdt_node;
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: Check for overlap in reserved memory regions

[Mitchel Humpherys]

On Mon, Sep 14 2015 at 02:21:04 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Sep 11, 2015 at 12:31 PM, Mitchel Humpherys
> <mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> Any overlap in the reserved memory regions (those specified in the
>> reserved-memory DT node) is a bug.  These bugs might go undetected as
>> long as the contested region isn't used simultaneously by multiple
>> software agents, which makes such bugs hard to debug.  Fix this by
>> printing a scary warning during boot if overlap is detected.

[...]

>> +
>> +static void __init __rmem_check_for_overlap(void)
>> +{
>> +       int i;
>> +
>> +       if (reserved_mem_count < 2)
>> +               return;
>> +
>> +       memcpy(sorted_reserved_mem, reserved_mem, sizeof(sorted_reserved_mem));
>> +       sort(sorted_reserved_mem, reserved_mem_count,
>> +            sizeof(sorted_reserved_mem[0]), __rmem_cmp, NULL);
>
> Why not just sort reserved_mem?

Yeah, I considered that but wasn't sure if it would break things in the
few places where the ordering of reserved_mem matters (like
__find_rmem).  Looking closer I think we're safe to sort reserved_mem
array directly to avoid the memcpy.  Will update in v2.

>
>> +       for (i = 0; i < reserved_mem_count - 1; i++) {
>> +               struct reserved_mem *this, *next;
>> +
>> +               this = &sorted_reserved_mem[i];
>> +               next = &sorted_reserved_mem[i + 1];
>> +               if (!(this->base && next->base))
>> +                       continue;
>> +               if (this->base + this->size > next->base) {
>> +                       phys_addr_t this_end, next_end;
>> +
>> +                       this_end = this->base + this->size;
>> +                       next_end = next->base + next->size;
>> +                       WARN(1, "Reserved mem: OVERLAP DETECTED!\n");
>> +                       pr_err("%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
>
> This seems overly verbose having both WARN and pr_err. I'd combine
> these. I don't think the stack trace from a WARN is too useful here
> given it is the DT file that users will need to go look at.

Yeah the reason for the WARN is to make it harder for this to slip by,
and I thought a BUG might be too heavy-handed.  I should be able to
squeeze the content of the pr_err into the WARN in v2.


-Mitch

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html