[PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[Dave Young]

Jan Stodola <jstodola@redhat.com> reported ppc64 root= is always added in kexec
kernel cmdline. But sometimes we need boot without root= for example we use
kexec to boot into installation initramfs image like below:
kexec --load vmlinuz --initrd=initrd.img --command-line=\
"inst.repo=http://<server>/<path>/Server/ppc64le/os/"

While creating dtb, in case there's no root= in user provided cmdline params 
kexec-tools will find the original root= param used in 1st kernel and pass it
to 2nd kernel. This caused that user have no way to remove root= cmdline.

Dropping that part of code so that one can get chance to kexec into 2nd kernel
without root= param. One can still provide root= in --command-line=""

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 kexec/fs2dt.c |   31 +------------------------------
 1 file changed, 1 insertion(+), 30 deletions(-)

--- kexec-tools.orig/kexec/fs2dt.c
+++ kexec-tools/kexec/fs2dt.c
@@ -579,8 +579,7 @@ 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.  */
+	/* Add cmdline to the second kernel. */
 	if (!strcmp(basename,"chosen/")) {
 		size_t result;
 		size_t cmd_len = 0;
@@ -594,36 +593,8 @@ static void putnode(void)
 			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) {
-			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 */

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

Re: [PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[Simon Horman]

Hi Dave,

On Fri, Oct 09, 2015 at 01:57:22PM +0800, Dave Young wrote:
> Jan Stodola <jstodola@redhat.com> reported ppc64 root= is always added in kexec
> kernel cmdline. But sometimes we need boot without root= for example we use
> kexec to boot into installation initramfs image like below:
> kexec --load vmlinuz --initrd=initrd.img --command-line=\
> "inst.repo=http://<server>/<path>/Server/ppc64le/os/"
> 
> While creating dtb, in case there's no root= in user provided cmdline params 
> kexec-tools will find the original root= param used in 1st kernel and pass it
> to 2nd kernel. This caused that user have no way to remove root= cmdline.
> 
> Dropping that part of code so that one can get chance to kexec into 2nd kernel
> without root= param. One can still provide root= in --command-line=""

I'm a little concerned about the backwards-compatibility implications of
this change. Though I agree the new behaviour is entirely sane perhaps
it should be activated via a new command line option.

Also, won't this affect other architectures that use DT.
I'm thinking about ARM. If so it might be good to tweak the changelog.

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

Re: [PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[Dave Young]

Hi, Simon

On 10/16/15 at 10:33am, Simon Horman wrote:
> Hi Dave,
> 
> On Fri, Oct 09, 2015 at 01:57:22PM +0800, Dave Young wrote:
> > Jan Stodola <jstodola@redhat.com> reported ppc64 root= is always added in kexec
> > kernel cmdline. But sometimes we need boot without root= for example we use
> > kexec to boot into installation initramfs image like below:
> > kexec --load vmlinuz --initrd=initrd.img --command-line=\
> > "inst.repo=http://<server>/<path>/Server/ppc64le/os/"
> > 
> > While creating dtb, in case there's no root= in user provided cmdline params 
> > kexec-tools will find the original root= param used in 1st kernel and pass it
> > to 2nd kernel. This caused that user have no way to remove root= cmdline.
> > 
> > Dropping that part of code so that one can get chance to kexec into 2nd kernel
> > without root= param. One can still provide root= in --command-line=""
> 
> I'm a little concerned about the backwards-compatibility implications of
> this change. Though I agree the new behaviour is entirely sane perhaps
> it should be activated via a new command line option.

I did not notice this problem until Jan reported it so I thouhgt that it is
just a hidden assumption, people might not notice it and use it that way.

For Fedora/RHEL kdump we will copy 1st kernel cmdline by default so one do
not need to use it. For kexec and other distributions I'm not sure..

Since it is more of a fix to original logic, personally I tend to not introducing
a new option. But if you think we need to add a new option I can do it. 

> 
> Also, won't this affect other architectures that use DT.
> I'm thinking about ARM. If so it might be good to tweak the changelog.

arm also uses the function, but I need verify the behavior. Will tune the changelog
after I confirming it.

Thanks
Dave

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

Re: [PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[Dave Young]

Hi, Simon

> 
> > 
> > Also, won't this affect other architectures that use DT.
> > I'm thinking about ARM. If so it might be good to tweak the changelog.
> 
> arm also uses the function, but I need verify the behavior. Will tune the changelog
> after I confirming it.

kexec on arm will have same problem when there's no external dtb being passed.
But kexec kernel failed to start up during my test there may be other problems.

I will work out a new patch for new option about skip adding root= from 1st kernel.

BTW, there's also a problem with external dtb --dtb option, I sent a patch to
revert the mmap patch from Michael, please review.

Thanks
Dave

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

Re: [PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[Dave Young]

On 10/23/15 at 11:19am, Dave Young wrote:
> Hi, Simon
> 
> > 
> > > 
> > > Also, won't this affect other architectures that use DT.
> > > I'm thinking about ARM. If so it might be good to tweak the changelog.
> > 
> > arm also uses the function, but I need verify the behavior. Will tune the changelog
> > after I confirming it.
> 
> kexec on arm will have same problem when there's no external dtb being passed.
> But kexec kernel failed to start up during my test there may be other problems.
> 
> I will work out a new patch for new option about skip adding root= from 1st kernel.

Simon, for the new option, I'm thinking to create a "--dt-no-old-root". It can be
a general option, or arch dependent one for arm and ppc64. What do you prefer?

Thanks
Dave

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

Re: [PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[Simon Horman]

On Fri, Oct 23, 2015 at 05:23:19PM +0800, Dave Young wrote:
> On 10/23/15 at 11:19am, Dave Young wrote:
> > Hi, Simon
> > 
> > > 
> > > > 
> > > > Also, won't this affect other architectures that use DT.
> > > > I'm thinking about ARM. If so it might be good to tweak the changelog.
> > > 
> > > arm also uses the function, but I need verify the behavior. Will tune the changelog
> > > after I confirming it.
> > 
> > kexec on arm will have same problem when there's no external dtb being passed.
> > But kexec kernel failed to start up during my test there may be other problems.
> > 
> > I will work out a new patch for new option about skip adding root= from 1st kernel.
> 
> Simon, for the new option, I'm thinking to create a "--dt-no-old-root". It can be
> a general option, or arch dependent one for arm and ppc64. What do you prefer?

Lets just make it arch dependent if it only changes the behaviour
on some architectures. The name sounds good to me.

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

Re: [PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[Dave Young]

On 10/26/15 at 01:32pm, Simon Horman wrote:
> On Fri, Oct 23, 2015 at 05:23:19PM +0800, Dave Young wrote:
> > On 10/23/15 at 11:19am, Dave Young wrote:
> > > Hi, Simon
> > > 
> > > > 
> > > > > 
> > > > > Also, won't this affect other architectures that use DT.
> > > > > I'm thinking about ARM. If so it might be good to tweak the changelog.
> > > > 
> > > > arm also uses the function, but I need verify the behavior. Will tune the changelog
> > > > after I confirming it.
> > > 
> > > kexec on arm will have same problem when there's no external dtb being passed.
> > > But kexec kernel failed to start up during my test there may be other problems.
> > > 
> > > I will work out a new patch for new option about skip adding root= from 1st kernel.
> > 
> > Simon, for the new option, I'm thinking to create a "--dt-no-old-root". It can be
> > a general option, or arch dependent one for arm and ppc64. What do you prefer?
> 
> Lets just make it arch dependent if it only changes the behaviour
> on some architectures. The name sounds good to me.

Ok, will do.

--
Thanks
Dave

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

Re: [PATCH] kexec-tools: do not copy 1st kernel root= param in fs2dt.c

[Simon Horman]

On Mon, Oct 19, 2015 at 04:22:46PM +0800, Dave Young wrote:
> Hi, Simon
> 
> On 10/16/15 at 10:33am, Simon Horman wrote:
> > Hi Dave,
> > 
> > On Fri, Oct 09, 2015 at 01:57:22PM +0800, Dave Young wrote:
> > > Jan Stodola <jstodola@redhat.com> reported ppc64 root= is always added in kexec
> > > kernel cmdline. But sometimes we need boot without root= for example we use
> > > kexec to boot into installation initramfs image like below:
> > > kexec --load vmlinuz --initrd=initrd.img --command-line=\
> > > "inst.repo=http://<server>/<path>/Server/ppc64le/os/"
> > > 
> > > While creating dtb, in case there's no root= in user provided cmdline params 
> > > kexec-tools will find the original root= param used in 1st kernel and pass it
> > > to 2nd kernel. This caused that user have no way to remove root= cmdline.
> > > 
> > > Dropping that part of code so that one can get chance to kexec into 2nd kernel
> > > without root= param. One can still provide root= in --command-line=""
> > 
> > I'm a little concerned about the backwards-compatibility implications of
> > this change. Though I agree the new behaviour is entirely sane perhaps
> > it should be activated via a new command line option.
> 
> I did not notice this problem until Jan reported it so I thouhgt that it
> is just a hidden assumption, people might not notice it and use it that
> way.
> 
> For Fedora/RHEL kdump we will copy 1st kernel cmdline by default so one
> do not need to use it. For kexec and other distributions I'm not sure..
> 
> Since it is more of a fix to original logic, personally I tend to not
> introducing a new option. But if you think we need to add a new option I
> can do it.

> > Also, won't this affect other architectures that use DT.
> > I'm thinking about ARM. If so it might be good to tweak the changelog.
> 
> arm also uses the function, but I need verify the behavior. Will tune the
> changelog after I confirming it.

Thanks

I'm still concerned that people may be relying on this behaviour.
But I agree it seems painful to add an option to get what
should have been the default.

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