[PATCH] kexec ppc64: fix misaligned cmdline

[PATCH] kexec ppc64: fix misaligned cmdline

[Michael Neuling]

If the cmdline changes between boots, we can get misalignment of the
bootargs entry, which in turn corrupts our device tree blob and hence
kills our kexec boot.  

Specifically, if the cmdline length was >= 8 before and the new cmdline
length is < 8, we can get corruption.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 kexec/arch/ppc64/fs2dt.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: kexec-tools-testing/kexec/arch/ppc64/fs2dt.c
===================================================================
--- kexec-tools-testing.orig/kexec/arch/ppc64/fs2dt.c
+++ kexec-tools-testing/kexec/arch/ppc64/fs2dt.c
@@ -197,6 +197,7 @@ static void putprops(char *fn, struct di
 	struct dirent *dp;
 	int i = 0, fd, len;
 	struct stat statbuf;
+	int dt_realigned = 0;
 
 	for (i = 0; i < numlist; i++) {
 		dp = nlist[i];
@@ -243,8 +244,10 @@ static void putprops(char *fn, struct di
 		*dt++ = len;
 		*dt++ = propnum(fn);
 
-		if ((len >= 8) && ((unsigned long)dt & 0x4))
+		if ((len >= 8) && ((unsigned long)dt & 0x4)){
 			dt++;
+			dt_realigned = 1;
+		}
 
 		fd = open(pathname, O_RDONLY);
 		if (fd == -1)
@@ -283,6 +286,8 @@ static void putprops(char *fn, struct di
 			strcat(local_cmdline, " ");
 			cmd_len = strlen(local_cmdline);
 			cmd_len = cmd_len + 1;
+			if (dt_realigned && cmd_len < 8)
+				dt--;
 			memcpy(dt, local_cmdline,cmd_len);
 			len = cmd_len;
 			*dt_len = cmd_len;

Re: [PATCH] kexec ppc64: fix misaligned cmdline

[Milton Miller]

On Jun 4, 2007, at 2:23 AM, Michael Neuling wrote:

> If the cmdline changes between boots, we can get misalignment of the
> bootargs entry, which in turn corrupts our device tree blob and hence
> kills our kexec boot.
...
> -		if ((len >= 8) && ((unsigned long)dt & 0x4))
> +		if ((len >= 8) && ((unsigned long)dt & 0x4)){
>  			dt++;
> +			dt_realigned = 1;
> +		}
>
>  		fd = open(pathname, O_RDONLY);
>  		if (fd == -1)
> @@ -283,6 +286,8 @@ static void putprops(char *fn, struct di
>  			strcat(local_cmdline, " ");
>  			cmd_len = strlen(local_cmdline);
>  			cmd_len = cmd_len + 1;
> +			if (dt_realigned && cmd_len < 8)
> +				dt--;
>  	

While this appears to fix the stated problem, did you explore my
suggestion of deleting and creating the property like we do for
the initrd base and size?

Deleting and recreating should also handle the case of no
boot-args in the original /chosen sub-tree (when the bootloader
didn't (need to) supply a command line).

milton

Re: [PATCH] kexec ppc64: fix misaligned cmdline

[Michael Neuling]

> While this appears to fix the stated problem, did you explore my
> suggestion of deleting and creating the property like we do for
> the initrd base and size?

Yeah, but it didn't seem "hapazard" enough for kexec. :-)

> Deleting and recreating should also handle the case of no boot-args in
> the original /chosen sub-tree (when the bootloader didn't (need to)
> supply a command line).

Yeah, we're screwed when the first boot has no bootargs.

Let me get the policy right.  We blow away the old command line except
for the "root=blah" item.  Correct?

Mikey

Re: [PATCH] kexec ppc64: fix misaligned cmdline

[David Gibson]

On Mon, Jun 04, 2007 at 05:23:45PM +1000, Miichael Neuling wrote:
> If the cmdline changes between boots, we can get misalignment of the
> bootargs entry, which in turn corrupts our device tree blob and hence
> kills our kexec boot.  
> 
> Specifically, if the cmdline length was >= 8 before and the new cmdline
> length is < 8, we can get corruption.

Hrm.  Have you considered using dtc for this conversion, rather than
this somewhat dubious looking fs2dt?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] kexec ppc64: fix misaligned cmdline

[Michael Neuling]

> > If the cmdline changes between boots, we can get misalignment of the
> > bootargs entry, which in turn corrupts our device tree blob and hence
> > kills our kexec boot.  
> > 
> > Specifically, if the cmdline length was >= 8 before and the new cmdline
> > length is < 8, we can get corruption.
> 
> Hrm.  Have you considered using dtc for this conversion, rather than
> this somewhat dubious looking fs2dt?

Numerous times.  

fs2dt needs to be converted to use the device tree library, but that
will take a lot more work than a 5 minute patch.

Mikey

Re: [PATCH] kexec ppc64: fix misaligned cmdline

[David Gibson]

On Tue, Jun 05, 2007 at 09:56:37AM +1000, Miichael Neuling wrote:
> > > If the cmdline changes between boots, we can get misalignment of the
> > > bootargs entry, which in turn corrupts our device tree blob and hence
> > > kills our kexec boot.  
> > > 
> > > Specifically, if the cmdline length was >= 8 before and the new cmdline
> > > length is < 8, we can get corruption.
> > 
> > Hrm.  Have you considered using dtc for this conversion, rather than
> > this somewhat dubious looking fs2dt?
> 
> Numerous times.  
> 
> fs2dt needs to be converted to use the device tree library, but that
> will take a lot more work than a 5 minute patch.


Well, sure.. but do you need fs2dt at all.  dtc has an "fs" input
mode..

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] kexec ppc64: fix misaligned cmdline

[Michael Neuling]

> On Tue, Jun 05, 2007 at 09:56:37AM +1000, Miichael Neuling wrote:
> > > > If the cmdline changes between boots, we can get misalignment of the
> > > > bootargs entry, which in turn corrupts our device tree blob and hence
> > > > kills our kexec boot.  
> > > > 
> > > > Specifically, if the cmdline length was >= 8 before and the new cmdline
> > > > length is < 8, we can get corruption.
> > > 
> > > Hrm.  Have you considered using dtc for this conversion, rather than
> > > this somewhat dubious looking fs2dt?
> > 
> > Numerous times.  
> > 
> > fs2dt needs to be converted to use the device tree library, but that
> > will take a lot more work than a 5 minute patch.
> 
> 
> Well, sure.. but do you need fs2dt at all.  dtc has an "fs" input
> mode..

As discussed offline, you can do this with the --devicetreeblob kexec
option.  The issue is the special cases like having to modify the kernel
cmdline options or initrd locations.

Mikey

[PATCH] kexec ppc64: fix misaligned cmdline

[Michael Neuling]

If the cmdline changes between boots, we can get misalignment of the
bootargs entry, which in turn corrupts our device tree blob and hence
kills our kexec boot.  Also, if there was no /chosen/bootargs
previously, we'd never add a new one. 

This ignores the bootargs while parsing the tree and inserts it later. 

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
This hopefully address issues raised by Milton.  

 kexec/arch/ppc64/fs2dt.c |   95 +++++++++++++++++++++++++++++++----------------
 1 file changed, 63 insertions(+), 32 deletions(-)

Index: kexec-tools-testing/kexec/arch/ppc64/fs2dt.c
===================================================================
--- kexec-tools-testing.orig/kexec/arch/ppc64/fs2dt.c
+++ kexec-tools-testing/kexec/arch/ppc64/fs2dt.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#define _GNU_SOURCE
 #include <sys/types.h>
 #include <sys/stat.h>
 
@@ -233,6 +234,12 @@ static void putprops(char *fn, struct di
 		    !reuse_initrd)
 				continue;
 
+		/* This property will be created later in putnode() So
+		 * ignore it now.
+		 */
+		if (!strcmp(dp->d_name, "bootargs"))
+			continue;
+
 		if (! S_ISREG(statbuf.st_mode))
 			continue;
 
@@ -257,38 +264,6 @@ static void putprops(char *fn, struct di
 
 		checkprop(fn, dt, len);
 
-		/* Get the cmdline from the device-tree and modify it */
-		if (!strcmp(dp->d_name, "bootargs")) {
-			int cmd_len;
-			char temp_cmdline[COMMAND_LINE_SIZE] = { "" };
-			char *param = NULL;
-			cmd_len = strlen(local_cmdline);
-			if (cmd_len != 0) {
-				param = strstr(local_cmdline, "crashkernel=");
-				if (param)
-					crash_param = 1;
-				param = strstr(local_cmdline, "root=");
-			}
-			if (!param) {
-				char *old_param;
-				memcpy(temp_cmdline, dt, len);
-				param = strstr(temp_cmdline, "root=");
-				if (param) {
-					old_param = strtok(param, " ");
-					if (cmd_len != 0)
-						strcat(local_cmdline, " ");
-					strcat(local_cmdline, old_param);
-				}
-			}
-			strcat(local_cmdline, " ");
-			cmd_len = strlen(local_cmdline);
-			cmd_len = cmd_len + 1;
-			memcpy(dt, local_cmdline,cmd_len);
-			len = cmd_len;
-			*dt_len = cmd_len;
-			fprintf(stderr, "Modified cmdline:%s\n", local_cmdline);
-		}
-
 		dt += (len + 3)/4;
 		if (!strcmp(dp->d_name, "reg") && usablemem_rgns.size)
 			add_usable_mem_property(fd, len);
@@ -385,6 +360,62 @@ static void putnode(void)
 		reserve(initrd_base, initrd_size);
 	}
 
+	/* Add cmdline to the second kernel.  Check to see if the new
+	 * cmdline has a root=.  If not, use the old root= cmdline.  */
+	if (!strcmp(basename,"/chosen/")) {
+		size_t cmd_len = 0;
+		char *param = NULL;
+
+		cmd_len = strlen(local_cmdline);
+		if (cmd_len != 0) {
+			param = strstr(local_cmdline, "crashkernel=");
+			if (param)
+				crash_param = 1;
+			/* does the new cmdline have a root= ? ... */
+			param = strstr(local_cmdline, "root=");
+		}
+
+		/* ... if not, grab root= from the old command line */
+		if (!param) {
+			char filename[MAXPATH];
+			FILE *fp;
+			char *last_cmdline = NULL;
+			char *old_param;
+
+			strcpy(filename, pathname);
+			strcat(filename, "bootargs");
+			fp = fopen(filename, "r");
+			if (fp) {
+				if (getline(&last_cmdline, &cmd_len, fp) == -1)
+					die("unable to read %s\n", filename);
+
+				param = strstr(last_cmdline, "root=");
+				if (param) {
+					old_param = strtok(param, " ");
+					if (cmd_len != 0)
+						strcat(local_cmdline, " ");
+					strcat(local_cmdline, old_param);
+				}
+			}
+			if (last_cmdline)
+				free(last_cmdline);
+		}
+		strcat(local_cmdline, " ");
+		cmd_len = strlen(local_cmdline);
+		cmd_len = cmd_len + 1;
+
+		/* add new bootargs */
+		*dt++ = 3;
+		*dt++ = cmd_len;
+		*dt++ = propnum("bootargs");
+		if ((cmd_len >= 8) && ((unsigned long)dt & 0x4))
+			dt++;
+		memcpy(dt, local_cmdline,cmd_len);
+		dt += (cmd_len + 3)/4;
+
+		fprintf(stderr, "Modified cmdline:%s\n", local_cmdline);
+	}
+
 	for (i=0; i < numlist; i++) {
 		dp = namelist[i];
 		strcpy(dn, dp->d_name);

Re: [PATCH] kexec ppc64: fix misaligned cmdline

[Geoff Levand]

Michael Neuling wrote:
> If the cmdline changes between boots, we can get misalignment of the
> bootargs entry, which in turn corrupts our device tree blob and hence
> kills our kexec boot.  Also, if there was no /chosen/bootargs
> previously, we'd never add a new one. 
> 
> This ignores the bootargs while parsing the tree and inserts it later. 
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Hi.

Acked-by: Geoff Levand <geoffrey.levand@am.sony.com>

I tested this with PS3 and it works as expected.  I added it to my
ps3-kexec-tools.git.  With this patch mainline kexec will work
with the PS3.

I still have a work-in-progress patch started by David Woodhouse
to support building a 32 bit kexec executable for PPC64 platforms.
I don't have any definite plan to work on it though.

-Geoff

Re: [PATCH] kexec ppc64: fix misaligned cmdline

[Horms]

On Thu, Jun 07, 2007 at 11:19:22AM +1000, Michael Neuling wrote:
> If the cmdline changes between boots, we can get misalignment of the
> bootargs entry, which in turn corrupts our device tree blob and hence
> kills our kexec boot.  Also, if there was no /chosen/bootargs
> previously, we'd never add a new one. 
> 
> This ignores the bootargs while parsing the tree and inserts it later. 
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Thanks, applied.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/