diff mbox

Fix crash due to processing "memory-controller" nodes as "memory"

Message ID 20150722091844.114ea023@kryten (mailing list archive)
State Not Applicable
Headers show

Commit Message

Anton Blanchard July 21, 2015, 11:18 p.m. UTC 
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(-)

Comments

Ian Munsie July 22, 2015, 12:36 a.m. UTC | #1 
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
Jeremy Kerr July 22, 2015, 12:39 a.m. UTC | #2 
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
Michael Neuling July 22, 2015, 1:32 a.m. UTC | #3 
On Wed, 2015-07-22 at 08:39 +0800, jeremy@ozlabs.au.ibm.com wrote:
> 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?

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
Anton Blanchard July 22, 2015, 5:15 a.m. UTC | #4 
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
diff mbox

Patch

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);