[PATCH] of: fdt: Check overlap of reserved memory regions

[PATCH] of: fdt: Check overlap of reserved memory regions

[Mårten Lindahl]

If a DT specified reserved memory region overlaps an already registered
reserved region no notification is made. Starting the system with
overlapped memory regions can make it very hard to debug what is going
wrong. This is specifically true in case the ramoops console intersects
with initrd since the console overwrites memory that is used for initrd,
which leads to memory corruption.

Highlight this by printing a message about overlapping memory regions.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/of/fdt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bdca35284ceb..c6b88a089b35 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -521,6 +521,11 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
 		size = dt_mem_next_cell(dt_root_size_cells, &prop);
 
+		if (size && memblock_is_reserved(base)) {
+			pr_warn("WARNING: 0x%08llx+0x%08llx overlaps reserved memory region\n",
+				(u64)base, (u64)size);
+		}
+
 		if (size &&
 		    early_init_dt_reserve_memory_arch(base, size, nomap) == 0)
 			pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
-- 
2.30.2

Re: [PATCH] of: fdt: Check overlap of reserved memory regions

[Rob Herring]

On Tue, Jan 11, 2022 at 6:25 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
> If a DT specified reserved memory region overlaps an already registered
> reserved region no notification is made. Starting the system with
> overlapped memory regions can make it very hard to debug what is going
> wrong. This is specifically true in case the ramoops console intersects
> with initrd since the console overwrites memory that is used for initrd,
> which leads to memory corruption.
>
> Highlight this by printing a message about overlapping memory regions.

Won't this be noisy if a region is described in both /memreserve/ and
/reserved-memory node?

>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
>  drivers/of/fdt.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index bdca35284ceb..c6b88a089b35 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -521,6 +521,11 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>                 base = dt_mem_next_cell(dt_root_addr_cells, &prop);
>                 size = dt_mem_next_cell(dt_root_size_cells, &prop);
>
> +               if (size && memblock_is_reserved(base)) {
> +                       pr_warn("WARNING: 0x%08llx+0x%08llx overlaps reserved memory region\n",
> +                               (u64)base, (u64)size);
> +               }
> +
>                 if (size &&
>                     early_init_dt_reserve_memory_arch(base, size, nomap) == 0)
>                         pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
> --
> 2.30.2
>

Re: [PATCH] of: fdt: Check overlap of reserved memory regions

[Marten Lindahl]

On Tue, Jan 11, 2022 at 07:34:00PM +0100, Rob Herring wrote:
> On Tue, Jan 11, 2022 at 6:25 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:

Hi Rob!
Thanks for looking at this.
> >
> > If a DT specified reserved memory region overlaps an already registered
> > reserved region no notification is made. Starting the system with
> > overlapped memory regions can make it very hard to debug what is going
> > wrong. This is specifically true in case the ramoops console intersects
> > with initrd since the console overwrites memory that is used for initrd,
> > which leads to memory corruption.
> >
> > Highlight this by printing a message about overlapping memory regions.
> 
> Won't this be noisy if a region is described in both /memreserve/ and
> /reserved-memory node?
> 
Yes, it can potentially be noisy if doing so. But I think notifying this
can be useful. Should it perhaps be a notification instead of a warning?

Kind regards
Mårten
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >  drivers/of/fdt.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index bdca35284ceb..c6b88a089b35 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -521,6 +521,11 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
> >                 base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> >                 size = dt_mem_next_cell(dt_root_size_cells, &prop);
> >
> > +               if (size && memblock_is_reserved(base)) {
> > +                       pr_warn("WARNING: 0x%08llx+0x%08llx overlaps reserved memory region\n",
> > +                               (u64)base, (u64)size);
> > +               }
> > +
> >                 if (size &&
> >                     early_init_dt_reserve_memory_arch(base, size, nomap) == 0)
> >                         pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
> > --
> > 2.30.2
> >

Re: [PATCH] of: fdt: Check overlap of reserved memory regions

[Stephen Boyd]

Quoting Marten Lindahl (2022-01-13 07:56:42)
> On Tue, Jan 11, 2022 at 07:34:00PM +0100, Rob Herring wrote:
> > On Tue, Jan 11, 2022 at 6:25 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
> Hi Rob!
> Thanks for looking at this.
> > >
> > > If a DT specified reserved memory region overlaps an already registered
> > > reserved region no notification is made. Starting the system with
> > > overlapped memory regions can make it very hard to debug what is going
> > > wrong. This is specifically true in case the ramoops console intersects
> > > with initrd since the console overwrites memory that is used for initrd,
> > > which leads to memory corruption.
> > >
> > > Highlight this by printing a message about overlapping memory regions.
> >
> > Won't this be noisy if a region is described in both /memreserve/ and
> > /reserved-memory node?
> >
> Yes, it can potentially be noisy if doing so. But I think notifying this
> can be useful. Should it perhaps be a notification instead of a warning?
>

Please don't print any message for /memreserve/ and /reserved-memory nodes
overlapping. On the chromebook at my desk we have overlapping
/memreserve/ and /reserved-memory. My understanding is that it's
redundant to have both, especially when a reserved-memory node has
'no-map', but it isn't forbidden. The /memreserve/ is like a no-map
/resreved-memory node without the phandle.

Given that initrd is special cased in drivers/of/fdt.c can the reserved
memory handling code look to see if it overlaps with the initrd region
and skip that /reserved-memory carveout? A warning could probably be
printed and ramoops should fail to probe.

Re: [PATCH] of: fdt: Check overlap of reserved memory regions

[Marten Lindahl]

On Thu, Jan 13, 2022 at 11:12:11PM +0100, Stephen Boyd wrote:
> Quoting Marten Lindahl (2022-01-13 07:56:42)
> > On Tue, Jan 11, 2022 at 07:34:00PM +0100, Rob Herring wrote:
> > > On Tue, Jan 11, 2022 at 6:25 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:
> >
> > Hi Rob!
> > Thanks for looking at this.
> > > >
> > > > If a DT specified reserved memory region overlaps an already registered
> > > > reserved region no notification is made. Starting the system with
> > > > overlapped memory regions can make it very hard to debug what is going
> > > > wrong. This is specifically true in case the ramoops console intersects
> > > > with initrd since the console overwrites memory that is used for initrd,
> > > > which leads to memory corruption.
> > > >
> > > > Highlight this by printing a message about overlapping memory regions.
> > >
> > > Won't this be noisy if a region is described in both /memreserve/ and
> > > /reserved-memory node?
> > >
> > Yes, it can potentially be noisy if doing so. But I think notifying this
> > can be useful. Should it perhaps be a notification instead of a warning?
> >

Hi Stephen!
> 
> Please don't print any message for /memreserve/ and /reserved-memory nodes
> overlapping. On the chromebook at my desk we have overlapping
> /memreserve/ and /reserved-memory. My understanding is that it's
> redundant to have both, especially when a reserved-memory node has
> 'no-map', but it isn't forbidden. The /memreserve/ is like a no-map
> /resreved-memory node without the phandle.
> 
> Given that initrd is special cased in drivers/of/fdt.c can the reserved
> memory handling code look to see if it overlaps with the initrd region
> and skip that /reserved-memory carveout? A warning could probably be
> printed and ramoops should fail to probe.

I understand if the check would spam on some system setups. So yes, I should
make the check less generic. The case which I describe with initrd and ramoops
is something that I think should be warned about.

But this would result in a very specific check for these two regions. So I'm
thinking, since the ramoops region is the one that will cause overwrites of
any other intersecting region, not necessarily just initrd, would it maybe
make sense to just add an extra check for ramoops and then print the warning?

And then still let ramoops run, as it depends on what memory part is
conflicting, and may not necessarily break the system.

Something like this?

if (!fdt_node_check_compatible(initial_boot_params,
			       node, "ramoops") &&
    size && memblock_is_reserved(base)) {
	pr_warn("WARNING: %s [0x%08llx+0x%08llx] overlaps reserved memory region\n",
		uname, (u64)base, (u64)size);
}

Any more thoughts?

Kind regards
Mårten