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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

root= cmdline modification in kexec (was 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?

OK Looks like we parse the new command line to see if there is a
'root=blah' parameter.  If yes, use that, otherwise we use what was on
the last boot.

Is this what other architectures are doing?  Is there any other special
cases we need to look out for?

Mikey

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: root= cmdline modification in kexec (was Re: [PATCH] kexec ppc64: fix misaligned cmdline )

[Mohan Kumar M]

On Tue, Jun 05, 2007 at 06:22:18PM +1000, Michael Neuling wrote:
> OK Looks like we parse the new command line to see if there is a
> 'root=blah' parameter.  If yes, use that, otherwise we use what was on
> the last boot.

For PPC64, if the user does not pass the kernel parameter, it is taken
from the existing kernel parameter. In this case "crashkernel=X@Y"
parameter is removed. If user explicitly gives the parameter, it is
used.

> 
> Is this what other architectures are doing?  Is there any other special
> cases we need to look out for?

I am not sure about x86, I hope they expect that user has to _pass_ the
kernel parameter. They don't copy the kernel parameter from current
kernel.

> 
> Mikey
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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









_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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/


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec