[PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Wang YanQing]

I get garbage output of /proc/cmdline and in dmesg when I
use kexec to load new kernel bzImage without append command
line like below:

kexec -l bzImage --initrd=initramfs.
This patch fix it.

Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 kexec/arch/i386/kexec-bzImage.c | 3 +++
 kexec/arch/i386/kexec-elf-x86.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/kexec/arch/i386/kexec-bzImage.c b/kexec/arch/i386/kexec-bzImage.c
index 99fd790..29f280d 100644
--- a/kexec/arch/i386/kexec-bzImage.c
+++ b/kexec/arch/i386/kexec-bzImage.c
@@ -435,6 +435,9 @@ int bzImage_load(int argc, char **argv, const char *buf, off_t len,
 	command_line_len = 0;
 	if (command_line) {
 		command_line_len = strlen(command_line) +1;
+	} else {
+	    command_line = strdup("\0");
+	    command_line_len = 1;
 	}
 	ramdisk_buf = 0;
 	if (ramdisk) {
diff --git a/kexec/arch/i386/kexec-elf-x86.c b/kexec/arch/i386/kexec-elf-x86.c
index e62ebcb..788a209 100644
--- a/kexec/arch/i386/kexec-elf-x86.c
+++ b/kexec/arch/i386/kexec-elf-x86.c
@@ -161,6 +161,9 @@ int elf_x86_load(int argc, char **argv, const char *buf, off_t len,
 	command_line_len = 0;
 	if (command_line) {
 		command_line_len = strlen(command_line) +1;
+	} else {
+	    command_line = strdup("\0");
+	    command_line_len = 1;
 	}
 
 	/* Need to append some command line parameters internally in case of
-- 
1.7.12.4.dirty

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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Zhang Yanfei]

于 2013年04月03日 17:43, Wang YanQing 写道:
> I get garbage output of /proc/cmdline and in dmesg when I
> use kexec to load new kernel bzImage without append command
> line like below:
> 
> kexec -l bzImage --initrd=initramfs.

Hello Wang

I tried this:

./build/sbin/kexec -l /boot/vmlinuz-2.6.32-279.el6.i686 --initrd=/boot/initramfs-2.6.32-279.el6.i686.img 

But nothing happened and it seemed the command loaded the kernel successfully.
Can you describe your situation more concretely, please?

Thanks
Zhang

> This patch fix it.
> 
> Signed-off-by: Wang YanQing <udknight@gmail.com>
> ---
>  kexec/arch/i386/kexec-bzImage.c | 3 +++
>  kexec/arch/i386/kexec-elf-x86.c | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/kexec/arch/i386/kexec-bzImage.c b/kexec/arch/i386/kexec-bzImage.c
> index 99fd790..29f280d 100644
> --- a/kexec/arch/i386/kexec-bzImage.c
> +++ b/kexec/arch/i386/kexec-bzImage.c
> @@ -435,6 +435,9 @@ int bzImage_load(int argc, char **argv, const char *buf, off_t len,
>  	command_line_len = 0;
>  	if (command_line) {
>  		command_line_len = strlen(command_line) +1;
> +	} else {
> +	    command_line = strdup("\0");
> +	    command_line_len = 1;
>  	}
>  	ramdisk_buf = 0;
>  	if (ramdisk) {
> diff --git a/kexec/arch/i386/kexec-elf-x86.c b/kexec/arch/i386/kexec-elf-x86.c
> index e62ebcb..788a209 100644
> --- a/kexec/arch/i386/kexec-elf-x86.c
> +++ b/kexec/arch/i386/kexec-elf-x86.c
> @@ -161,6 +161,9 @@ int elf_x86_load(int argc, char **argv, const char *buf, off_t len,
>  	command_line_len = 0;
>  	if (command_line) {
>  		command_line_len = strlen(command_line) +1;
> +	} else {
> +	    command_line = strdup("\0");
> +	    command_line_len = 1;
>  	}
>  
>  	/* Need to append some command line parameters internally in case of


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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Wang YanQing]

On Sat, Apr 06, 2013 at 01:52:09PM +0800, Zhang Yanfei wrote:
> 于 2013年04月03日 17:43, Wang YanQing 写道:
> > I get garbage output of /proc/cmdline and in dmesg when I
> > use kexec to load new kernel bzImage without append command
> > line like below:
> > 
> > kexec -l bzImage --initrd=initramfs.
> 
> Hello Wang
> 
> I tried this:
> 
> ./build/sbin/kexec -l /boot/vmlinuz-2.6.32-279.el6.i686 --initrd=/boot/initramfs-2.6.32-279.el6.i686.img 
> 
> But nothing happened and it seemed the command loaded the kernel successfully.
> Can you describe your situation more concretely, please?
Yes, it will load the kernel successfully, but when you use kexec -e to run into
the loaded kernel, in the new kernel context, you will get garbage output when 
execute `cat /proc/cmdline`, and you also can see the garbage in "Kernel command
line" from dmesg. 

I am sure you will see this issue, because when user pass no cmdline, and cmdline_len
will because zero, so the line below in setup_linux_bootloader_parameters_high will
not work:
cmdline_ptr[cmdline_len - 1] = '\0';

Thanks

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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Zhang Yanfei]

于 2013年04月07日 09:01, Wang YanQing 写道:
> On Sat, Apr 06, 2013 at 01:52:09PM +0800, Zhang Yanfei wrote:
>> 于 2013年04月03日 17:43, Wang YanQing 写道:
>>> I get garbage output of /proc/cmdline and in dmesg when I
>>> use kexec to load new kernel bzImage without append command
>>> line like below:
>>>
>>> kexec -l bzImage --initrd=initramfs.
>>
>> Hello Wang
>>
>> I tried this:
>>
>> ./build/sbin/kexec -l /boot/vmlinuz-2.6.32-279.el6.i686 --initrd=/boot/initramfs-2.6.32-279.el6.i686.img 
>>
>> But nothing happened and it seemed the command loaded the kernel successfully.
>> Can you describe your situation more concretely, please?
> Yes, it will load the kernel successfully, but when you use kexec -e to run into
> the loaded kernel, in the new kernel context, you will get garbage output when 
> execute `cat /proc/cmdline`, and you also can see the garbage in "Kernel command
> line" from dmesg. 

With no commandline, can the new kernel boot?
I tried in my box and the new kernel just panicked for it cannot
find a root= argument in its commandline.

Thanks
Zhang

> 
> I am sure you will see this issue, because when user pass no cmdline, and cmdline_len
> will because zero, so the line below in setup_linux_bootloader_parameters_high will
> not work:
> cmdline_ptr[cmdline_len - 1] = '\0';
> 
> Thanks
> 


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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Wang YanQing]

On Sun, Apr 07, 2013 at 01:54:58PM +0800, Zhang Yanfei wrote:
> With no commandline, can the new kernel boot?
> I tried in my box and the new kernel just panicked for it cannot
> find a root= argument in its commandline.
I am sure the kernel boot, 
rootfs_initcall(populate_rootfs) in initramfs.c
well populate the root fs, and there is a /init
in initramfs, this /init will mount the really 
root device, and the system is running.
 

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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Wang YanQing]

On Sun, Apr 07, 2013 at 05:35:40PM +0800, Wang YanQing wrote:
> On Sun, Apr 07, 2013 at 01:54:58PM +0800, Zhang Yanfei wrote:
> > With no commandline, can the new kernel boot?
> > I tried in my box and the new kernel just panicked for it cannot
> > find a root= argument in its commandline.
> I am sure the kernel boot, 
> rootfs_initcall(populate_rootfs) in initramfs.c
> well populate the root fs, and there is a /init
> in initramfs, this /init will mount the really 
> root device, and the system is running.

Your kernel panic, because the below line in init/main.c failed:

if (sys_access((const char __user *) ramdisk_execute_command, 0) != 0)

kernel then run into prepare_namespace, but prepare_namespace failed too,
then the kernel panic.

I don't know why, but if rootfs_initcall(populate_rootfs) works ok,
kernel don't need to call prepare_namespace, because the decompressed 
initramfs will become the root fs and sys_access will success.

Sorry for I forget to mention my test kernel is v3.8.6, but if my memory don't lie me,
I can boot v2.6.32 without a root= parameter, we had use v2.6.32 as product kernel
still more than one year two years ago.

Thanks


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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Zhang Yanfei]

于 2013年04月08日 09:08, Wang YanQing 写道:
> On Sun, Apr 07, 2013 at 05:35:40PM +0800, Wang YanQing wrote:
>> On Sun, Apr 07, 2013 at 01:54:58PM +0800, Zhang Yanfei wrote:
>>> With no commandline, can the new kernel boot?
>>> I tried in my box and the new kernel just panicked for it cannot
>>> find a root= argument in its commandline.
>> I am sure the kernel boot, 
>> rootfs_initcall(populate_rootfs) in initramfs.c
>> well populate the root fs, and there is a /init
>> in initramfs, this /init will mount the really 
>> root device, and the system is running.
> 
> Your kernel panic, because the below line in init/main.c failed:
> 
> if (sys_access((const char __user *) ramdisk_execute_command, 0) != 0)
> 
> kernel then run into prepare_namespace, but prepare_namespace failed too,
> then the kernel panic.
> 
> I don't know why, but if rootfs_initcall(populate_rootfs) works ok,
> kernel don't need to call prepare_namespace, because the decompressed 
> initramfs will become the root fs and sys_access will success.
> 
> Sorry for I forget to mention my test kernel is v3.8.6, 

I tried 3.8.0 kernel. Unfortunately, panicked again. For some reason, I didn't
see the panic message.

but if my memory don't lie me,
> I can boot v2.6.32 without a root= parameter, we had use v2.6.32 as product kernel
> still more than one year two years ago.

Sigh, I tried in a real box and a kvm machine. Both panicked with no root= argument
message. I don't know why.

Anyway, Just from the code, your patch didn't fix all the possible place.
do_bzImage64_load may also call setup_linux_bootloader_parameters_high with
a null commandline. So why not change the check in
setup_linux_bootloader_parameters_high.

--------------------------
diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
index 454fad6..6eb2e6e 100644
--- a/kexec/arch/i386/x86-linux-setup.c
+++ b/kexec/arch/i386/x86-linux-setup.c
@@ -116,7 +116,8 @@ void setup_linux_bootloader_parameters_high(
        /* Fill in the command line */
        if (cmdline_len > COMMAND_LINE_SIZE) {
                cmdline_len = COMMAND_LINE_SIZE;
-       }
+       } else if (cmdline_len == 0)
+               return;
        cmdline_ptr = ((char *)real_mode) + cmdline_offset;
        memcpy(cmdline_ptr, cmdline, cmdline_len);
        cmdline_ptr[cmdline_len - 1] = '\0';

Thanks
Zhang

> 
> Thanks
> 
> 
> _______________________________________________
> 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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Wang YanQing]

On Mon, Apr 08, 2013 at 11:35:25AM +0800, Zhang Yanfei wrote:
> I tried 3.8.0 kernel. Unfortunately, panicked again. For some reason, I didn't
> see the panic message.
> 
> >but if my memory don't lie me,
> > I can boot v2.6.32 without a root= parameter, we had use v2.6.32 as product kernel
> > still more than one year two years ago.
> 
> Sigh, I tried in a real box and a kvm machine. Both panicked with no root= argument
> message. I don't know why.

It maybe your CONFIG relation problem, I guess. I have booted ok more than 3 times.
Are your have CONFIG_BLK_DEV_INITRD=y in .config? And your cpio format initramfs
has init script in root directory? And your init script will auto-mount your really
device right before switch_root into it?

> Anyway, Just from the code, your patch didn't fix all the possible place.
> do_bzImage64_load may also call setup_linux_bootloader_parameters_high with
> a null commandline. So why not change the check in
> setup_linux_bootloader_parameters_high.
Your are right. But your patch is wrong too.
See below.

> 
> --------------------------
> diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> index 454fad6..6eb2e6e 100644
> --- a/kexec/arch/i386/x86-linux-setup.c
> +++ b/kexec/arch/i386/x86-linux-setup.c
> @@ -116,7 +116,8 @@ void setup_linux_bootloader_parameters_high(
>         /* Fill in the command line */
>         if (cmdline_len > COMMAND_LINE_SIZE) {
>                 cmdline_len = COMMAND_LINE_SIZE;
> -       }
> +       } else if (cmdline_len == 0)
> +               return;
Can't just return, we must set the string termination guard like below:

+ else if (cmdline_len == 0)
+       cmdline_len = 1;

If you agreed, maybe I can resend the v2 patch.
>         cmdline_ptr = ((char *)real_mode) + cmdline_offset;
>         memcpy(cmdline_ptr, cmdline, cmdline_len);
>         cmdline_ptr[cmdline_len - 1] = '\0';
> 

Thanks.

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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Zhang Yanfei]

于 2013年04月08日 11:53, Wang YanQing 写道:
> On Mon, Apr 08, 2013 at 11:35:25AM +0800, Zhang Yanfei wrote:
>> I tried 3.8.0 kernel. Unfortunately, panicked again. For some reason, I didn't
>> see the panic message.
>>
>>> but if my memory don't lie me,
>>> I can boot v2.6.32 without a root= parameter, we had use v2.6.32 as product kernel
>>> still more than one year two years ago.
>>
>> Sigh, I tried in a real box and a kvm machine. Both panicked with no root= argument
>> message. I don't know why.
> 
> It maybe your CONFIG relation problem, I guess. I have booted ok more than 3 times.
> Are your have CONFIG_BLK_DEV_INITRD=y in .config? And your cpio format initramfs
> has init script in root directory? And your init script will auto-mount your really
> device right before switch_root into it?
> 
>> Anyway, Just from the code, your patch didn't fix all the possible place.
>> do_bzImage64_load may also call setup_linux_bootloader_parameters_high with
>> a null commandline. So why not change the check in
>> setup_linux_bootloader_parameters_high.
> Your are right. But your patch is wrong too.
> See below.
> 
>>
>> --------------------------
>> diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
>> index 454fad6..6eb2e6e 100644
>> --- a/kexec/arch/i386/x86-linux-setup.c
>> +++ b/kexec/arch/i386/x86-linux-setup.c
>> @@ -116,7 +116,8 @@ void setup_linux_bootloader_parameters_high(
>>         /* Fill in the command line */
>>         if (cmdline_len > COMMAND_LINE_SIZE) {
>>                 cmdline_len = COMMAND_LINE_SIZE;
>> -       }
>> +       } else if (cmdline_len == 0)
>> +               return;
> Can't just return, we must set the string termination guard like below:

I think this is ok for we have filled all the real_mode buffer with 0.

But I just found this fix still has problem...

Let's go through the code, take do_bzImage_load for example.

234         setup_size = kern16_size_needed + command_line_len +
235                          PURGATORY_CMDLINE_SIZE;
236         real_mode = xmalloc(setup_size);
237         memset(real_mode, 0, setup_size);
......
300         setup_linux_bootloader_parameters(info, real_mode, setup_base,
301                 kern16_size_needed, command_line, command_line_len,
302                 initrd, initrd_len);
......
369         cmdline_end = setup_base + kern16_size_needed + command_line_len - 1;
370         elf_rel_set_symbol(&info->rhdr, "cmdline_end", &cmdline_end,
371                            sizeof(unsigned long));

if no commandline and we set command_line_len to 0, line 369 is still wrong. It
will corrupt the kernel16 buf.

So your original patch is ok if we also set command_line_len to 1 and make
cmdline = "\0" in bzImage64_load.

> 
> + else if (cmdline_len == 0)
> +       cmdline_len = 1;
> 
> If you agreed, maybe I can resend the v2 patch.
>>         cmdline_ptr = ((char *)real_mode) + cmdline_offset;
>>         memcpy(cmdline_ptr, cmdline, cmdline_len);
>>         cmdline_ptr[cmdline_len - 1] = '\0';
>>
> 
> Thanks.
> 
> _______________________________________________
> 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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Wang YanQing]

On Mon, Apr 08, 2013 at 12:33:40PM +0800, Zhang Yanfei wrote:
> I think this is ok for we have filled all the real_mode buffer with 0.
I don't think so, it must be somethings wrong, if we had filled all
the real_mode buffer with 0, why do we need my patch to set 
the string termination guard '\0'? 

Does '\0' equal zero, right?

Thanks.

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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Zhang Yanfei]

于 2013年04月08日 14:07, Wang YanQing 写道:
> On Mon, Apr 08, 2013 at 12:33:40PM +0800, Zhang Yanfei wrote:
>> I think this is ok for we have filled all the real_mode buffer with 0.
> I don't think so, it must be somethings wrong, if we had filled all
> the real_mode buffer with 0, why do we need my patch to set 
> the string termination guard '\0'? 

real_mode = kernel16 buf + commandline + purgatory commandline.

At first, we filled all real_mode buffer with 0, then we assigned values
to kernel16 buf just in the head of real_mode.
And next, we copied the commandline just after the kernel16 buf. Usually,
when we copy a string into a buffer, we will set the string termination
guard '\0' right after the string in the buffer for safety, I think.

As your patch, If we have no commandline, and just assign it to '\0'. It
prevents the problems to happen below:

In setup_linux_bootloader_parameters_high

120         cmdline_ptr = ((char *)real_mode) + cmdline_offset;
121         memcpy(cmdline_ptr, cmdline, cmdline_len);
122         cmdline_ptr[cmdline_len - 1] = '\0';

if cmdline_len == 0, Line 122 will corrupt kernel16 buf just before the commandline.

And in do_bzImage_load, for example,

369         cmdline_end = setup_base + kern16_size_needed + command_line_len - 1;
370         elf_rel_set_symbol(&info->rhdr, "cmdline_end", &cmdline_end,
371                            sizeof(unsigned long));

Line 369 will go wrong, too.

> 
> Does '\0' equal zero, right?

Yeah, if we write '\0', the value store into the char buf is zero, I think.

Correct me if I am wrong, please.

Thanks
Zhang

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

Re: [PATCH] i386:kexec-bzImage: Use "\0" as command line instead of empty command line

[Wang YanQing]

On Mon, Apr 08, 2013 at 11:35:25AM +0800, Zhang Yanfei wrote:
> I tried 3.8.0 kernel. Unfortunately, panicked again. For some reason, I didn't
> see the panic message.
Maybe it hasn't paniced. Could you ping it from another machine? I meet 
an situation which looks like panic after I run kexec -e, but it isn't 
except the screen is hang, I can't see any output from screen, but I can 
login in through ssh from another machine, of course, it is another story.

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