From: Rob Landley <[email protected]>
Run init: HOME=/ TERM=linux /init
Signed-off-by: Rob Landley <[email protected]>
---
init/main.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/init/main.c b/init/main.c
index 130376ec10ba..80b06566852b 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1328,15 +1328,16 @@ static void __init do_pre_smp_initcalls(void)
static int run_init_process(const char *init_filename)
{
const char *const *p;
+ char buf[512], *s = buf;
argv_init[0] = init_filename;
- pr_info("Run %s as init process\n", init_filename);
- pr_debug(" with arguments:\n");
- for (p = argv_init; *p; p++)
- pr_debug(" %s\n", *p);
- pr_debug(" with environment:\n");
- for (p = envp_init; *p; p++)
- pr_debug(" %s\n", *p);
+
+ for (p = (void *)envp_init; *p; p++)
+ s += sprintf(s, "%.*s ", (int)(sizeof(buf)-(s-buf)), *p);
+ for (p = (void *)argv_init; *p; p++)
+ s += sprintf(s, "%.*s ", (int)(sizeof(buf)-(s-buf)), *p);
+ pr_info("Run init: %s\n", buf);
+
return kernel_execve(init_filename, argv_init, envp_init);
}
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: ac0e958a00eabda16984e87d64cc2dc38e59b10e ("[PATCH] Collate "run init" message to one line with prefixed var assignments")
url: https://github.com/0day-ci/linux/commits/Rob-Landley/Collate-run-init-message-to-one-line-with-prefixed-var-assignments/20201111-234005
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git eccc876724927ff3b9ff91f36f7b6b159e948f0c
in testcase: trinity
version: trinity-static-i386-x86_64-f93256fb_2019-08-28
with following parameters:
runtime: 300s
test-description: Trinity is a linux system call fuzz tester.
test-url: http://codemonkey.org.uk/projects/trinity/
on test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 8G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+----------------------------------------------------------------------------------------+------------+------------+
| | eccc876724 | ac0e958a00 |
+----------------------------------------------------------------------------------------+------------+------------+
| Kernel_panic-not_syncing:stack-protector:Kernel_stack_is_corrupted_in:run_init_process | 0 | 8 |
+----------------------------------------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 18.936653] Freeing unused kernel image (initmem) memory: 1028K
[ 18.937375] Write protecting kernel text and read-only data: 28964k
[ 18.938014] rodata_test: all tests were successful
[ 18.939555] Failed to set sysctl parameter 'kernel.softlockup_panic=1': parameter not found
[ 18.940519] Run init: HOME=/ TERM=linux user=lkp job=/lkp/jobs/scheduled/vm-snb-i386-113/trinity-300s-openwrt-i386-generic-20190428.cgz-ac0e958a00eabda16984e87d64cc2dc38e59b10e-20201112-21005-197i8k2-2.yaml ARCH=i386 kconfig=i386-randconfig-r016-20201111 branch=linux-review/Rob-Landley/Collate-run-init-message-to-one-line-with-prefixed-var-assignments/20201111-234005 commit=ac0e958a00eabda16984e87d64cc2dc38e59b10e BOOT_IMAGE=/pkg/linux/i386-randconfig-r016-20201111/gcc-9/ac0e958a00eabda16984e87d64cc2dc38e59b10e/vmlinuz-5.10.0-rc
[ 18.945937] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: run_init_process+0x129/0x131
[ 18.946930] CPU: 0 PID: 1 Comm: swapper Not tainted 5.10.0-rc3-00105-gac0e958a00ea #1
[ 18.947639] Call Trace:
[ 18.947884] ? dump_stack+0x45/0x63
[ 18.948212] ? panic+0x196/0x5c1
[ 18.948512] ? __stack_chk_fail+0x2d/0x30
[ 18.948878] ? run_init_process+0x129/0x131
[ 18.949273] ? run_init_process+0x129/0x131
[ 18.949663] ? try_to_run_init_process+0x1a/0x66
[ 18.950094] ? rest_init+0x259/0x259
[ 18.950428] ? kernel_init+0x1ab/0x267
[ 18.950775] ? ret_from_fork+0x1c/0x30
[ 18.951183] Kernel Offset: disabled
Kboot worker: lkp-server01
Elapsed time: 60
To reproduce:
# build kernel
cd linux
cp config-5.10.0-rc3-00105-gac0e958a00ea .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=i386 olddefconfig prepare modules_prepare bzImage
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks,
Oliver Sang
On 11/12/20 1:11 AM, kernel test robot wrote:
>
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-9):
Blah, switched from strlcpy to sprintf due to the lack of spaces and didn't
adjust the size.
(And yes, the compiler's lifetime analysis should free the stack space before
the tail call, and I'd assume exec restarts the stack anyway.)
Second-attempt-by: Rob Landley <[email protected]>
---
init/main.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/init/main.c b/init/main.c
index 130376ec10ba..e92320816ef8 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1328,15 +1328,16 @@ static void __init do_pre_smp_initcalls(void)
static int run_init_process(const char *init_filename)
{
const char *const *p;
+ char buf[512], *s = buf;
argv_init[0] = init_filename;
- pr_info("Run %s as init process\n", init_filename);
- pr_debug(" with arguments:\n");
- for (p = argv_init; *p; p++)
- pr_debug(" %s\n", *p);
- pr_debug(" with environment:\n");
- for (p = envp_init; *p; p++)
- pr_debug(" %s\n", *p);
+
+ for (p = (void *)envp_init; *p; p++)
+ s += sprintf(s, "%.*s ", (int)(sizeof(buf)-(s-buf)-2), *p);
+ for (p = (void *)argv_init; *p; p++)
+ s += sprintf(s, "%.*s ", (int)(sizeof(buf)-(s-buf)-2), *p);
+ pr_info("Run init: %s\n", buf);
+
return kernel_execve(init_filename, argv_init, envp_init);
}
From: Rob Landley
> Sent: 12 November 2020 12:46
>
> On 11/12/20 1:11 AM, kernel test robot wrote:
> >
> > Greeting,
> >
> > FYI, we noticed the following commit (built with gcc-9):
>
> Blah, switched from strlcpy to sprintf due to the lack of spaces and didn't
> adjust the size.
>
> (And yes, the compiler's lifetime analysis should free the stack space before
> the tail call, and I'd assume exec restarts the stack anyway.)
>
> Second-attempt-by: Rob Landley <[email protected]>
> ---
>
> init/main.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index 130376ec10ba..e92320816ef8 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1328,15 +1328,16 @@ static void __init do_pre_smp_initcalls(void)
> static int run_init_process(const char *init_filename)
> {
> const char *const *p;
> + char buf[512], *s = buf;
>
> argv_init[0] = init_filename;
> - pr_info("Run %s as init process\n", init_filename);
> - pr_debug(" with arguments:\n");
> - for (p = argv_init; *p; p++)
> - pr_debug(" %s\n", *p);
> - pr_debug(" with environment:\n");
> - for (p = envp_init; *p; p++)
> - pr_debug(" %s\n", *p);
> +
> + for (p = (void *)envp_init; *p; p++)
> + s += sprintf(s, "%.*s ", (int)(sizeof(buf)-(s-buf)-2), *p);
> + for (p = (void *)argv_init; *p; p++)
> + s += sprintf(s, "%.*s ", (int)(sizeof(buf)-(s-buf)-2), *p);
> + pr_info("Run init: %s\n", buf);
> +
Why not use scnprintf() as:
len += scnprintf(buf + len, 256 - len, " %s", *p);
or even:
s = buf + sizeof buf;
len = sizeof buf;
...
len -= scnprintf(s - len, len, " %s", *p);
and remove the " " before the %s in the final pr_info().
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 11/12/20 7:49 AM, David Laight wrote:
> From: Rob Landley
>> Sent: 12 November 2020 12:46
>>
>> On 11/12/20 1:11 AM, kernel test robot wrote:
>>>
>>> Greeting,
>>>
>>> FYI, we noticed the following commit (built with gcc-9):
>>
>> Blah, switched from strlcpy to sprintf due to the lack of spaces and didn't
>> adjust the size.
>>
>> (And yes, the compiler's lifetime analysis should free the stack space before
>> the tail call, and I'd assume exec restarts the stack anyway.)
This is why I didn't put anything like that in the first submission. (I knew
better, and did it anyway...)
>> Second-attempt-by: Rob Landley <[email protected]>
>> ---
>>
>> init/main.c | 15 ++++++++-------
>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/init/main.c b/init/main.c
>> index 130376ec10ba..e92320816ef8 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -1328,15 +1328,16 @@ static void __init do_pre_smp_initcalls(void)
>> static int run_init_process(const char *init_filename)
>> {
>> const char *const *p;
>> + char buf[512], *s = buf;
>>
>> argv_init[0] = init_filename;
>> - pr_info("Run %s as init process\n", init_filename);
>> - pr_debug(" with arguments:\n");
>> - for (p = argv_init; *p; p++)
>> - pr_debug(" %s\n", *p);
>> - pr_debug(" with environment:\n");
>> - for (p = envp_init; *p; p++)
>> - pr_debug(" %s\n", *p);
>> +
>> + for (p = (void *)envp_init; *p; p++)
>> + s += sprintf(s, "%.*s ", (int)(sizeof(buf)-(s-buf)-2), *p);
>> + for (p = (void *)argv_init; *p; p++)
>> + s += sprintf(s, "%.*s ", (int)(sizeof(buf)-(s-buf)-2), *p);
>> + pr_info("Run init: %s\n", buf);
>> +
>
> Why not use scnprintf() as:
> len += scnprintf(buf + len, 256 - len, " %s", *p);
Because what I did worked for me?
The buffer size isn't 256, sizeof() means if the buffer size changes the code
automatically adjusts and the -2 gets constant folded at compile time anyway,
you've proposed switching from a posix function to a kernel-specific function
for no obvious benefit, it's the same number of arguments and other than 2 bytes
in a string constant you've just swapped s-buf for buf+len, I didn't want to dig
into whether passing rdinit= could have an empty environment so the "skip a
space" has no space to skip and thus you skip the null terminator so I just left
one harmlessly trailing on the end, if I wanted to get FANCY I'd measure and
allocate space then free it after printing...
I could go on, but that's about as much bikeshedding as I have the stomach for
right now on two for loops calling print statements, thanks.
> or even:
> s = buf + sizeof buf;
> len = sizeof buf;
> ...
> len -= scnprintf(s - len, len, " %s", *p);
>
> and remove the " " before the %s in the final pr_info().
Feel free to submit your own patch...?
I don't really expect this to get merged. It's not like I cc'd any humans. My
latest thingied-by tag is not an approved entry in
Documentation/process/submitting-patches.rst and I didn't go through all 27
steps in Documentation/process/submit-checklist.rst because I'm not part of the
fulltime kernel political clique and I don't bother to fight them anymore. It's
just a small thing that annoyed me and I mostly posted it here so when some
clown sues us for shipping a modified kernel I can cost them more money by
pointing their lawyers at the patch on the list's web archive. (I could do so on
a website I maintain, but then I'd have to track it and dowanna.)
I was only ever involved here as a hobbyist. The Linux Foundation is currently
holding a "conference dedicated to driving collaboration and innovation in
financial services" with "featured speakers" including the Managing Director of
Goldman Sachs, the Founder of the Alliance for Innovative Regulation, the former
CIO of Deutsche Bank, Red Hat's Director of Financial Services Strategy, and
whatever "Open Source Wonk, Azure Office of the CTO, Microsoft" means.
No, I did not make that up, they spammed me about it as part of their perpetual
fundraising strategy to sell for-profit conference tickets:
https://events.linuxfoundation.org/open-source-strategy-forum/
Rob