| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <CBCBEB391604FEindou.takao@jp.fujitsu.com>
Date: Fri, 25 Mar 2011 18:07:52 -0400
From: Takao Indoh <indou.takao@...fujitsu.com>
To: Milton Miller <miltonm@....com>, linux-kernel@...r.kernel.org,
kexec@...ts.infradead.org
Cc: Jens Axboe <axboe@...nel.dk>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Ingo Molnar <mingo@...e.hu>, Vivek Goyal <vgoyal@...hat.com>,
WANG Cong <xiyou.wangcong@...il.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH] generic-ipi: Initialize call_single_queue before enabling interrupt
On Fri, 25 Mar 2011 11:48:21 -0600, Milton Miller wrote:
>On Fri, 25 Mar 2011 about 11:52:20 -0400, Takao Indoh wrote:
>>
>> Hi all,
>>
>> Actually this is a v2 patch but I post with new subject.
>> The first post is here.
>>
>> [PATCH][KDUMP] Ignore spurious IPI
>> http://www.gossamer-threads.com/lists/linux/kernel/1356389
>>
>> The problem I found is that kdump(2nd kernel) sometimes hangs up due to
>> pending IPI from 1st kernel. Kernel panic occurs because IPI comes
>> before call_single_queue is initialized, so this patch moves
>> init_call_single_data() so that it can be called before
>> local_irq_enable().
>>
>
>This is good background, but I generally prefer changelogs to start
>by describing what is being changed and then proceed on to why, and
>then the history of the patch and discussion.
>
>> The details of the problem is below.
>>
>> (1)
>> 2nd kernel boot up
>>
>> (2)
>> A pending IPI from 1st kernel comes after unmasking interrupts at the
>> following point.
>>
>> asmlinkage void __init start_kernel(void)
>> {
>> (snip)
>> time_init();
>> profile_init();
>> if (!irqs_disabled())
>> printk(KERN_CRIT "start_kernel(): bug: interrupts were "
>> "enabled early\n");
>> early_boot_irqs_disabled = false;
>> local_irq_enable(); <=======================================HERE
>>
>
>This could be described more concisely as "when irqs are first enabled
>in start_kernel()".
>
>> (3)
>> Kernel tries to handle the interrupt, but call_single_queue is not
>> initialized yet at this point. As a result, in the
>> generic_smp_call_function_single_interrupt(), NULL pointer dereference
>> occurs when list_replace_init() tries to access &q->list.next.
>>
>> Any comments would be appreciated.
>>
>> Signed-off-by: Takao Indoh <indou.takao@...fujitsu.com>
>> ---
>> include/linux/smp.h | 1 +
>> init/main.c | 1 +
>> kernel/smp.c | 18 +++++++++++-------
>> 3 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/smp.h b/include/linux/smp.h
>> index 6dc95ca..0b81bba 100644
>> --- a/include/linux/smp.h
>> +++ b/include/linux/smp.h
>> @@ -114,6 +114,7 @@ int on_each_cpu(smp_call_func_t func, void *info, int
>> wait);
>> void smp_prepare_boot_cpu(void);
>>
>> extern unsigned int setup_max_cpus;
>> +void init_call_single_data(void);
>>
>
>This no longer applys (although it did to 2.6.38).
>
>> #else /* !SMP */
>>
>
>I was going to point out that this will fail on up, but it looks
>like ba8dd03ac09f51a69c154b8cb508b701d713a2cd (Full conversion to
>early_initcall() interface, remove old interface) only found 2 of the
>previous 3 occurrances.
>
>However, this does point out that not all CONFIG_SMP compiles are
>CONFIG_USE_GENERIC_SMP_HELPERS, and you need to adjust accordingly.
>
>[ for other reviewers no this is not a simple revert of a hunk of that
>2.6.27 change, this is moving the code yet earlier ].
>
>> diff --git a/init/main.c b/init/main.c
>> index 33c37c3..02a7617 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -631,6 +631,7 @@ asmlinkage void __init start_kernel(void)
>> printk(KERN_CRIT "start_kernel(): bug: interrupts were "
>> "enabled early\n");
>> early_boot_irqs_disabled = false;
>> + init_call_single_data();
>> local_irq_enable();
>>
>
>1) this is too late, the point of the printk and the state variable is
>to make sure interrupts are enabled exactly when we expect for the
>first time.
Ok, so the following place is correct?
profile_init();
+ call_function_init();
if (!irqs_disabled())
printk(KERN_CRIT "start_kernel(): bug: interrupts were "
"enabled early\n");
early_boot_irqs_disabled = false;
local_irq_enable();
>
>2) please follow the naming conventions and name this xxx_init. Actually
>I prefer xxx be the name of the subsystem (call_function) instead of
>the name of the data structure (call_single_data).
>
>> /* Interrupts are enabled now so all GFP allocations are safe. */
>> diff --git a/kernel/smp.c b/kernel/smp.c
>> index 7cbd0f2..f119610 100644
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -74,9 +74,19 @@ static struct notifier_block __cpuinitdata
>> hotplug_cfd_notifier = {
>> .notifier_call = hotplug_cfd,
>> };
>>
>> -static int __cpuinit init_call_single_data(void)
>> +static int __cpuinit init_hotplug_cfd(void)
>> {
>> void *cpu = (void *)(long)smp_processor_id();
>> +
>> + hotplug_cfd(&hotplug_cfd_notifier, CPU_UP_PREPARE, cpu);
>> + register_cpu_notifier(&hotplug_cfd_notifier);
>> +
>> + return 0;
>> +}
>> +early_initcall(init_hotplug_cfd);
>> +
>
>Why did you seperate this from the rest of the function? I haven't
>tested the patch but don't why this needs to be delayed. If there
>is some reason I don't see, please describe it in the change log.
There is not special reason. I just thought I should minimize the change
to prevent regression because I cannot do hotplug test. Ok, next time
I'll just change the name from init_call_single_data to
call_function_init instead of separating it.
It seems that Peter Zijlstra added this hotplug code at 8969a5ed, so I
added him to Cc.
Thanks for your review.
Thanks,
Takao Indoh
>
>> +void __init init_call_single_data(void)
>> +{
>> int i;
>>
>> for_each_possible_cpu(i) {
>> @@ -85,13 +95,7 @@ static int __cpuinit init_call_single_data(void)
>> raw_spin_lock_init(&q->lock);
>> INIT_LIST_HEAD(&q->list);
>> }
>> -
>> - hotplug_cfd(&hotplug_cfd_notifier, CPU_UP_PREPARE, cpu);
>> - register_cpu_notifier(&hotplug_cfd_notifier);
>> -
>> - return 0;
>> }
>> -early_initcall(init_call_single_data);
>>
>> /*
>> * csd_lock/csd_unlock used to serialize access to per-cpu csd resources
>>
>
>milton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists