[PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

If the system has a PCI device with a memory-controller device node,
kexec-lite would spew hundreds of double free warnings and eventually
segfault. This would result in a "kexec load failed" message from
petitboot.

This was due to kexec_memory_map() searching for "memory" nodes, but
actually matching any node that started with "memory", including these
"memory-controller" nodes. This patch changes the search to look for
nodes starting with "memory@", which should only match memory nodes.

An example of a device tree that can trigger this bug is as follows:

{
	pciex@3fffe40000000 {
		...
		pci@0 {
			#address-cells = <0x3>;
			#size-cells = <0x2>;
			...
			memory-controller@0 {
				reg = <0x10000 0x0 0x0 0x0 0x0>;
				...
			};
		};
	};
};

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 kexec_memory_map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kexec_memory_map.c b/kexec_memory_map.c
index fc1b7af..7f18de7 100644
--- a/kexec_memory_map.c
+++ b/kexec_memory_map.c
@@ -192,7 +192,7 @@ void kexec_memory_map(void *fdt, int reserve_initrd)
 
 		name = fdt_get_name(fdt, nodeoffset, NULL);
 
-		if (!name || strncmp(name, "memory", strlen("memory")))
+		if (!name || strncmp(name, "memory@", strlen("memory@")))
 			continue;
 
 		reg = fdt_getprop(fdt, nodeoffset, "reg", &len);
-- 
2.1.4

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Ian Munsie]

Whoops - subject line was supposed to say "kexec-lite: ..."

-Ian

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Anton Blanchard]

Hi Ian,

> From: Ian Munsie <imunsie@au1.ibm.com>
> 
> If the system has a PCI device with a memory-controller device node,
> kexec-lite would spew hundreds of double free warnings and eventually
> segfault. This would result in a "kexec load failed" message from
> petitboot.
> 
> This was due to kexec_memory_map() searching for "memory" nodes, but
> actually matching any node that started with "memory", including these
> "memory-controller" nodes. This patch changes the search to look for
> nodes starting with "memory@", which should only match memory nodes.

Nice catch! I wonder if we should be checking for device_type
"memory". Ben?

Anton

> An example of a device tree that can trigger this bug is as follows:
> 
> {
> 	pciex@3fffe40000000 {
> 		...
> 		pci@0 {
> 			#address-cells = <0x3>;
> 			#size-cells = <0x2>;
> 			...
> 			memory-controller@0 {
> 				reg = <0x10000 0x0 0x0 0x0 0x0>;
> 				...
> 			};
> 		};
> 	};
> };
> 
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> ---
>  kexec_memory_map.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kexec_memory_map.c b/kexec_memory_map.c
> index fc1b7af..7f18de7 100644
> --- a/kexec_memory_map.c
> +++ b/kexec_memory_map.c
> @@ -192,7 +192,7 @@ void kexec_memory_map(void *fdt, int
> reserve_initrd) 
>  		name = fdt_get_name(fdt, nodeoffset, NULL);
>  
> -		if (!name || strncmp(name, "memory",
> strlen("memory")))
> +		if (!name || strncmp(name, "memory@",
> strlen("memory@"))) continue;
>  
>  		reg = fdt_getprop(fdt, nodeoffset, "reg", &len);

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Benjamin Herrenschmidt]

On Tue, 2015-07-21 at 12:45 +1000, Anton Blanchard wrote:
> Hi Ian,
> 
> > From: Ian Munsie <imunsie@au1.ibm.com>
> > 
> > If the system has a PCI device with a memory-controller device node,
> > kexec-lite would spew hundreds of double free warnings and eventually
> > segfault. This would result in a "kexec load failed" message from
> > petitboot.
> > 
> > This was due to kexec_memory_map() searching for "memory" nodes, but
> > actually matching any node that started with "memory", including these
> > "memory-controller" nodes. This patch changes the search to look for
> > nodes starting with "memory@", which should only match memory nodes.
> 
> Nice catch! I wonder if we should be checking for device_type
> "memory". Ben?

Or at least check for the nodes at the root of the DT only.

Cheers,
Ben.

> Anton
> 
> > An example of a device tree that can trigger this bug is as follows:
> > 
> > {
> > 	pciex@3fffe40000000 {
> > 		...
> > 		pci@0 {
> > 			#address-cells = <0x3>;
> > 			#size-cells = <0x2>;
> > 			...
> > 			memory-controller@0 {
> > 				reg = <0x10000 0x0 0x0 0x0 0x0>;
> > 				...
> > 			};
> > 		};
> > 	};
> > };
> > 
> > Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> > ---
> >  kexec_memory_map.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kexec_memory_map.c b/kexec_memory_map.c
> > index fc1b7af..7f18de7 100644
> > --- a/kexec_memory_map.c
> > +++ b/kexec_memory_map.c
> > @@ -192,7 +192,7 @@ void kexec_memory_map(void *fdt, int
> > reserve_initrd) 
> >  		name = fdt_get_name(fdt, nodeoffset, NULL);
> >  
> > -		if (!name || strncmp(name, "memory",
> > strlen("memory")))
> > +		if (!name || strncmp(name, "memory@",
> > strlen("memory@"))) continue;
> >  
> >  		reg = fdt_getprop(fdt, nodeoffset, "reg", &len);

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Michael Ellerman]

On Tue, 2015-07-21 at 12:45 +1000, Anton Blanchard wrote:
> Hi Ian,
> 
> > From: Ian Munsie <imunsie@au1.ibm.com>
> > 
> > If the system has a PCI device with a memory-controller device node,
> > kexec-lite would spew hundreds of double free warnings and eventually
> > segfault. This would result in a "kexec load failed" message from
> > petitboot.
> > 
> > This was due to kexec_memory_map() searching for "memory" nodes, but
> > actually matching any node that started with "memory", including these
> > "memory-controller" nodes. This patch changes the search to look for
> > nodes starting with "memory@", which should only match memory nodes.
> 
> Nice catch! I wonder if we should be checking for device_type
> "memory". Ben?

Yes. That's what Linux does.

cheers

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Anton Blanchard]

Hi,

> > Nice catch! I wonder if we should be checking for device_type
> > "memory". Ben?
> 
> Yes. That's what Linux does.

Ian: I made that change, and slightly modified your commit message.
Look ok?

Anton
--

[PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

If the system has a PCI device with a memory-controller device node,
kexec-lite would spew hundreds of double free warnings and eventually
segfault. This would result in a "kexec load failed" message from
petitboot.

This was due to kexec_memory_map() searching for "memory" nodes, but
actually matching any node that started with "memory", including these
"memory-controller" nodes. This patch changes the search to look for
nodes with a device_type of "memory", which should only match memory
nodes.

An example of a device tree that can trigger this bug is as follows:

{
	pciex@3fffe40000000 {
		...
		pci@0 {
			#address-cells = <0x3>;
			#size-cells = <0x2>;
			...
			memory-controller@0 {
				reg = <0x10000 0x0 0x0 0x0 0x0>;
				...
			};
		};
	};
};

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
Signed-off-by: Anton Blanchard <anton@samba.org>
---
 kexec_memory_map.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kexec_memory_map.c b/kexec_memory_map.c
index fc1b7af..6103590 100644
--- a/kexec_memory_map.c
+++ b/kexec_memory_map.c
@@ -182,7 +182,7 @@ void kexec_memory_map(void *fdt, int reserve_initrd)
 	}
 
 	while (1) {
-		const char *name;
+		const char *type;
 		int len;
 		const fdt64_t *reg;
 
@@ -190,9 +190,9 @@ void kexec_memory_map(void *fdt, int reserve_initrd)
 		if (nodeoffset < 0)
 			break;
 
-		name = fdt_get_name(fdt, nodeoffset, NULL);
+		type = fdt_getprop(fdt, nodeoffset, "device_type", NULL);
 
-		if (!name || strncmp(name, "memory", strlen("memory")))
+		if (!type || strcmp(type, "memory"))
 			continue;
 
 		reg = fdt_getprop(fdt, nodeoffset, "reg", &len);
-- 
2.1.0

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Ian Munsie]

Excerpts from Anton Blanchard's message of 2015-07-22 09:18:44 +1000:
> Hi,
> 
> > > Nice catch! I wonder if we should be checking for device_type
> > > "memory". Ben?
> > 
> > Yes. That's what Linux does.
> 
> Ian: I made that change, and slightly modified your commit message.
> Look ok?

Looks good to me :)

Cheers,
-Ian

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Anton Blanchard]

Hi Ian,

> > > > Nice catch! I wonder if we should be checking for device_type
> > > > "memory". Ben?
> > > 
> > > Yes. That's what Linux does.
> > 
> > Ian: I made that change, and slightly modified your commit message.
> > Look ok?
> 
> Looks good to me :)

Excellent, I just pushed the fix.

Anton

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Jeremy Kerr]

Hi Anton,

> [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

Looks good to me. If you apply this to your kexec-lite repo, I'll update
op-build to use the new version, and send the merge requests for op-build.

I'd expect we'd have those changes upstream in the next couple of days.
Xilinx folks: how are you consuming op-build? Do you need a new PNOR
image, or are you constructing those yourself?

Regards,


Jeremy

Re: [PATCH] Fix crash due to processing "memory-controller" nodes as "memory"

[Michael Neuling]

On Wed, 2015-07-22 at 08:39 +0800, jeremy@ozlabs.au.ibm.com wrote:
> Hi Anton,
>=20
> > [PATCH] Fix crash due to processing "memory-controller" nodes as "memor=
y"
>=20
> Looks good to me. If you apply this to your kexec-lite repo, I'll update
> op-build to use the new version, and send the merge requests for op-build=
.
>=20
> I'd expect we'd have those changes upstream in the next couple of days.
> Xilinx folks: how are you consuming op-build? Do you need a new PNOR
> image, or are you constructing those yourself?

jk, Sonal (from Xilinx) is on a Tulleta not an openpower box.  So we
need to go through the IBM process to get the firmware updated rather
than op-build.

Mikey