* kdump: crash_kexec()-smp_send_stop() race in panic
@ 2011-10-24 14:55 Michael Holzheu
2011-10-24 15:14 ` Eric W. Biederman
0 siblings, 1 reply; 14+ messages in thread
From: Michael Holzheu @ 2011-10-24 14:55 UTC (permalink / raw)
To: Vivek Goyal
Cc: ebiederm, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel
Hello Vivek,
In our tests we ran into the following scenario:
Two CPUs have called panic at the same time. The first CPU called
crash_kexec() and the second CPU called smp_send_stop() in panic()
before crash_kexec() finished on the first CPU. So the second CPU
stopped the first CPU and therefore kdump failed.
1st CPU:
panic()->crash_kexec()->mutex_trylock(&kexec_mutex)-> do kdump
2nd CPU:
panic()->crash_kexec()->kexec_mutex already held by 1st CPU
->smp_send_stop()-> stop CPU 1 (stop kdump)
How should we fix this problem? One possibility could be to do
smp_send_stop() before we call crash_kexec().
What do you think?
Michael
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-24 14:55 kdump: crash_kexec()-smp_send_stop() race in panic Michael Holzheu @ 2011-10-24 15:14 ` Eric W. Biederman 2011-10-24 15:23 ` Américo Wang 0 siblings, 1 reply; 14+ messages in thread From: Eric W. Biederman @ 2011-10-24 15:14 UTC (permalink / raw) To: holzheu; +Cc: Vivek Goyal, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: > Hello Vivek, > > In our tests we ran into the following scenario: > > Two CPUs have called panic at the same time. The first CPU called > crash_kexec() and the second CPU called smp_send_stop() in panic() > before crash_kexec() finished on the first CPU. So the second CPU > stopped the first CPU and therefore kdump failed. > > 1st CPU: > panic()->crash_kexec()->mutex_trylock(&kexec_mutex)-> do kdump > > 2nd CPU: > panic()->crash_kexec()->kexec_mutex already held by 1st CPU > ->smp_send_stop()-> stop CPU 1 (stop kdump) > > How should we fix this problem? One possibility could be to do > smp_send_stop() before we call crash_kexec(). > > What do you think? smp_send_stop is insufficiently reliable to be used before crash_kexec. My first reaction would be to test oops_in_progress and wait until oops_in_progress == 1 before calling smp_send_stop. Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-24 15:14 ` Eric W. Biederman @ 2011-10-24 15:23 ` Américo Wang 2011-10-24 17:07 ` Eric W. Biederman 0 siblings, 1 reply; 14+ messages in thread From: Américo Wang @ 2011-10-24 15:23 UTC (permalink / raw) To: Eric W. Biederman Cc: holzheu, Vivek Goyal, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel On Mon, Oct 24, 2011 at 11:14 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: > >> Hello Vivek, >> >> In our tests we ran into the following scenario: >> >> Two CPUs have called panic at the same time. The first CPU called >> crash_kexec() and the second CPU called smp_send_stop() in panic() >> before crash_kexec() finished on the first CPU. So the second CPU >> stopped the first CPU and therefore kdump failed. >> >> 1st CPU: >> panic()->crash_kexec()->mutex_trylock(&kexec_mutex)-> do kdump >> >> 2nd CPU: >> panic()->crash_kexec()->kexec_mutex already held by 1st CPU >> ->smp_send_stop()-> stop CPU 1 (stop kdump) >> >> How should we fix this problem? One possibility could be to do >> smp_send_stop() before we call crash_kexec(). >> >> What do you think? > > smp_send_stop is insufficiently reliable to be used before crash_kexec. > > My first reaction would be to test oops_in_progress and wait until > oops_in_progress == 1 before calling smp_send_stop. > +1 One of my colleague mentioned the same problem with me inside RH, given the fact that the race condition window is small, it would not be easy to reproduce this scenario. Thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-24 15:23 ` Américo Wang @ 2011-10-24 17:07 ` Eric W. Biederman 2011-10-24 17:33 ` Vivek Goyal 2011-10-25 8:44 ` Michael Holzheu 0 siblings, 2 replies; 14+ messages in thread From: Eric W. Biederman @ 2011-10-24 17:07 UTC (permalink / raw) To: Américo Wang Cc: holzheu, Vivek Goyal, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel Américo Wang <xiyou.wangcong@gmail.com> writes: > On Mon, Oct 24, 2011 at 11:14 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: >> >>> Hello Vivek, >>> >>> In our tests we ran into the following scenario: >>> >>> Two CPUs have called panic at the same time. The first CPU called >>> crash_kexec() and the second CPU called smp_send_stop() in panic() >>> before crash_kexec() finished on the first CPU. So the second CPU >>> stopped the first CPU and therefore kdump failed. >>> >>> 1st CPU: >>> panic()->crash_kexec()->mutex_trylock(&kexec_mutex)-> do kdump >>> >>> 2nd CPU: >>> panic()->crash_kexec()->kexec_mutex already held by 1st CPU >>> ->smp_send_stop()-> stop CPU 1 (stop kdump) >>> >>> How should we fix this problem? One possibility could be to do >>> smp_send_stop() before we call crash_kexec(). >>> >>> What do you think? >> >> smp_send_stop is insufficiently reliable to be used before crash_kexec. >> >> My first reaction would be to test oops_in_progress and wait until >> oops_in_progress == 1 before calling smp_send_stop. >> > > +1 > > One of my colleague mentioned the same problem with me inside > RH, given the fact that the race condition window is small, it would > not be easy to reproduce this scenario. As for reproducing it I have a hunch you could hack up something horrible with smp_call_function and kprobes. On a little more reflection we can't wait until oops_in_progress goes to 1 before calling smp_send_stop. Because if crash_kexec is not involved nothing we will never call smp_send_stop. So my second thought is to introduce another atomic variable panic_in_progress, visible only in panic. The cpu that sets increments panic_in_progress can call smp_send_stop. The rest of the cpus can just go into a busy wait. That should stop nasty fights about who is going to come out of smp_send_stop first. Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-24 17:07 ` Eric W. Biederman @ 2011-10-24 17:33 ` Vivek Goyal 2011-10-24 22:24 ` Seiji Aguchi 2011-10-25 8:44 ` Michael Holzheu 1 sibling, 1 reply; 14+ messages in thread From: Vivek Goyal @ 2011-10-24 17:33 UTC (permalink / raw) To: Eric W. Biederman Cc: Américo Wang, holzheu, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel On Mon, Oct 24, 2011 at 10:07:19AM -0700, Eric W. Biederman wrote: > Américo Wang <xiyou.wangcong@gmail.com> writes: > > > On Mon, Oct 24, 2011 at 11:14 PM, Eric W. Biederman > > <ebiederm@xmission.com> wrote: > >> Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: > >> > >>> Hello Vivek, > >>> > >>> In our tests we ran into the following scenario: > >>> > >>> Two CPUs have called panic at the same time. The first CPU called > >>> crash_kexec() and the second CPU called smp_send_stop() in panic() > >>> before crash_kexec() finished on the first CPU. So the second CPU > >>> stopped the first CPU and therefore kdump failed. > >>> > >>> 1st CPU: > >>> panic()->crash_kexec()->mutex_trylock(&kexec_mutex)-> do kdump > >>> > >>> 2nd CPU: > >>> panic()->crash_kexec()->kexec_mutex already held by 1st CPU > >>> ->smp_send_stop()-> stop CPU 1 (stop kdump) > >>> > >>> How should we fix this problem? One possibility could be to do > >>> smp_send_stop() before we call crash_kexec(). > >>> > >>> What do you think? > >> > >> smp_send_stop is insufficiently reliable to be used before crash_kexec. > >> > >> My first reaction would be to test oops_in_progress and wait until > >> oops_in_progress == 1 before calling smp_send_stop. > >> > > > > +1 > > > > One of my colleague mentioned the same problem with me inside > > RH, given the fact that the race condition window is small, it would > > not be easy to reproduce this scenario. > > As for reproducing it I have a hunch you could hack up something > horrible with smp_call_function and kprobes. > > > On a little more reflection we can't wait until oops_in_progress goes > to 1 before calling smp_send_stop. Because if crash_kexec is not > involved nothing we will never call smp_send_stop. > > So my second thought is to introduce another atomic variable > panic_in_progress, visible only in panic. The cpu that sets > increments panic_in_progress can call smp_send_stop. The rest of > the cpus can just go into a busy wait. That should stop nasty > fights about who is going to come out of smp_send_stop first. Introducing panic_on_oops atomic sounds good. Thanks Vivek ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-24 17:33 ` Vivek Goyal @ 2011-10-24 22:24 ` Seiji Aguchi 2011-10-25 8:33 ` Michael Holzheu 0 siblings, 1 reply; 14+ messages in thread From: Seiji Aguchi @ 2011-10-24 22:24 UTC (permalink / raw) To: Vivek Goyal, Eric W. Biederman Cc: Américo Wang, holzheu@linux.vnet.ibm.com, akpm@linux-foundation.org, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, kexec@lists.infradead.org, linux-kernel@vger.kernel.org Hi, >> >>> 1st CPU: >> >>> panic()->crash_kexec()->mutex_trylock(&kexec_mutex)-> do kdump >> >>> >> >>> 2nd CPU: >> >>> panic()->crash_kexec()->kexec_mutex already held by 1st CPU >> >>> ->smp_send_stop()-> stop CPU 1 (stop kdump) >> >>> >> >>> How should we fix this problem? One possibility could be to do >> >>> smp_send_stop() before we call crash_kexec(). http://lkml.org/lkml/2010/9/16/353 I developed a patch solving this issue one year ago. (Just adding local_irq_disable in kexec path.) I hope this helps. Seiji ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-24 22:24 ` Seiji Aguchi @ 2011-10-25 8:33 ` Michael Holzheu 0 siblings, 0 replies; 14+ messages in thread From: Michael Holzheu @ 2011-10-25 8:33 UTC (permalink / raw) To: Seiji Aguchi Cc: Vivek Goyal, Eric W. Biederman, Américo Wang, akpm@linux-foundation.org, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, kexec@lists.infradead.org, linux-kernel@vger.kernel.org Hello Seiji, On Mon, 2011-10-24 at 18:24 -0400, Seiji Aguchi wrote: > Hi, > > >> >>> 1st CPU: > >> >>> panic()->crash_kexec()->mutex_trylock(&kexec_mutex)-> do kdump > >> >>> > >> >>> 2nd CPU: > >> >>> panic()->crash_kexec()->kexec_mutex already held by 1st CPU > >> >>> ->smp_send_stop()-> stop CPU 1 (stop kdump) > >> >>> > >> >>> How should we fix this problem? One possibility could be to do > >> >>> smp_send_stop() before we call crash_kexec(). > > http://lkml.org/lkml/2010/9/16/353 > > I developed a patch solving this issue one year ago. > (Just adding local_irq_disable in kexec path.) This won't work (at least on s390) because smp_send_stop() will also stop CPUs that have interrupts disabled. Michael ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-24 17:07 ` Eric W. Biederman 2011-10-24 17:33 ` Vivek Goyal @ 2011-10-25 8:44 ` Michael Holzheu 2011-10-25 12:04 ` Eric W. Biederman 1 sibling, 1 reply; 14+ messages in thread From: Michael Holzheu @ 2011-10-25 8:44 UTC (permalink / raw) To: Eric W. Biederman Cc: Américo Wang, Vivek Goyal, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel Hello Eric, On Mon, 2011-10-24 at 10:07 -0700, Eric W. Biederman wrote: [snip] > So my second thought is to introduce another atomic variable > panic_in_progress, visible only in panic. The cpu that sets > increments panic_in_progress can call smp_send_stop. The rest of > the cpus can just go into a busy wait. That should stop nasty > fights about who is going to come out of smp_send_stop first. So this is a spinlock, no? What about the following patch: --- kernel/panic.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) --- a/kernel/panic.c +++ b/kernel/panic.c @@ -59,6 +59,7 @@ EXPORT_SYMBOL(panic_blink); */ NORET_TYPE void panic(const char * fmt, ...) { + static DEFINE_SPINLOCK(panic_lock); static char buf[1024]; va_list args; long i, i_next = 0; @@ -68,8 +69,12 @@ NORET_TYPE void panic(const char * fmt, * It's possible to come here directly from a panic-assertion and * not have preempt disabled. Some functions called from here want * preempt to be disabled. No point enabling it later though... + * + * Only one CPU is allowed to execute the panic code. For multiple + * parallel invocations of panic all other CPUs will wait on the + * panic_lock. They are stopped afterwards by smp_send_stop(). */ - preempt_disable(); + spin_lock(&panic_lock); console_verbose(); bust_spinlocks(1); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-25 8:44 ` Michael Holzheu @ 2011-10-25 12:04 ` Eric W. Biederman 2011-10-25 14:54 ` Vivek Goyal 2011-10-25 14:58 ` Michael Holzheu 0 siblings, 2 replies; 14+ messages in thread From: Eric W. Biederman @ 2011-10-25 12:04 UTC (permalink / raw) To: holzheu Cc: Américo Wang, Vivek Goyal, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: > Hello Eric, > > On Mon, 2011-10-24 at 10:07 -0700, Eric W. Biederman wrote: > > [snip] > >> So my second thought is to introduce another atomic variable >> panic_in_progress, visible only in panic. The cpu that sets >> increments panic_in_progress can call smp_send_stop. The rest of >> the cpus can just go into a busy wait. That should stop nasty >> fights about who is going to come out of smp_send_stop first. > > So this is a spinlock, no? What about the following patch: Do we want both panic printks? We really only need the mutual exclusion starting just before smp_send_stop so that is where I would be inclined to put it. But yeah something like the below should work. Eric > --- > kernel/panic.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -59,6 +59,7 @@ EXPORT_SYMBOL(panic_blink); > */ > NORET_TYPE void panic(const char * fmt, ...) > { > + static DEFINE_SPINLOCK(panic_lock); > static char buf[1024]; > va_list args; > long i, i_next = 0; > @@ -68,8 +69,12 @@ NORET_TYPE void panic(const char * fmt, > * It's possible to come here directly from a panic-assertion and > * not have preempt disabled. Some functions called from here want > * preempt to be disabled. No point enabling it later though... > + * > + * Only one CPU is allowed to execute the panic code. For multiple > + * parallel invocations of panic all other CPUs will wait on the > + * panic_lock. They are stopped afterwards by smp_send_stop(). > */ > - preempt_disable(); > + spin_lock(&panic_lock); > > console_verbose(); > bust_spinlocks(1); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-25 12:04 ` Eric W. Biederman @ 2011-10-25 14:54 ` Vivek Goyal 2011-10-25 14:58 ` Michael Holzheu 1 sibling, 0 replies; 14+ messages in thread From: Vivek Goyal @ 2011-10-25 14:54 UTC (permalink / raw) To: Eric W. Biederman Cc: holzheu, Américo Wang, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel On Tue, Oct 25, 2011 at 05:04:57AM -0700, Eric W. Biederman wrote: > Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: > > > Hello Eric, > > > > On Mon, 2011-10-24 at 10:07 -0700, Eric W. Biederman wrote: > > > > [snip] > > > >> So my second thought is to introduce another atomic variable > >> panic_in_progress, visible only in panic. The cpu that sets > >> increments panic_in_progress can call smp_send_stop. The rest of > >> the cpus can just go into a busy wait. That should stop nasty > >> fights about who is going to come out of smp_send_stop first. > > > > So this is a spinlock, no? What about the following patch: > Do we want both panic printks? > I guess having printk() from from both the panics would be nice. > We really only need the mutual exclusion starting just before > smp_send_stop so that is where I would be inclined to put it. > How about something just before crash_kexec()? I think there is not much point two cpus trying to execute crash_kexec() together. Thanks Vivek > But yeah something like the below should work. > > Eric > > > > --- > > kernel/panic.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > --- a/kernel/panic.c > > +++ b/kernel/panic.c > > @@ -59,6 +59,7 @@ EXPORT_SYMBOL(panic_blink); > > */ > > NORET_TYPE void panic(const char * fmt, ...) > > { > > + static DEFINE_SPINLOCK(panic_lock); > > static char buf[1024]; > > va_list args; > > long i, i_next = 0; > > @@ -68,8 +69,12 @@ NORET_TYPE void panic(const char * fmt, > > * It's possible to come here directly from a panic-assertion and > > * not have preempt disabled. Some functions called from here want > > * preempt to be disabled. No point enabling it later though... > > + * > > + * Only one CPU is allowed to execute the panic code. For multiple > > + * parallel invocations of panic all other CPUs will wait on the > > + * panic_lock. They are stopped afterwards by smp_send_stop(). > > */ > > - preempt_disable(); > > + spin_lock(&panic_lock); > > > > console_verbose(); > > bust_spinlocks(1); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-25 12:04 ` Eric W. Biederman 2011-10-25 14:54 ` Vivek Goyal @ 2011-10-25 14:58 ` Michael Holzheu 2011-10-25 15:08 ` Vivek Goyal 1 sibling, 1 reply; 14+ messages in thread From: Michael Holzheu @ 2011-10-25 14:58 UTC (permalink / raw) To: Eric W. Biederman Cc: Américo Wang, Vivek Goyal, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel On Tue, 2011-10-25 at 05:04 -0700, Eric W. Biederman wrote: > Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: > > > Hello Eric, > > > > On Mon, 2011-10-24 at 10:07 -0700, Eric W. Biederman wrote: > > > > [snip] > > > >> So my second thought is to introduce another atomic variable > >> panic_in_progress, visible only in panic. The cpu that sets > >> increments panic_in_progress can call smp_send_stop. The rest of > >> the cpus can just go into a busy wait. That should stop nasty > >> fights about who is going to come out of smp_send_stop first. > > > > So this is a spinlock, no? What about the following patch: > Do we want both panic printks? Ok, good point. We proably should not change that. > We really only need the mutual exclusion starting just before > smp_send_stop so that is where I would be inclined to put it. I think to fix the race, at least we have the get the lock before we call crash_kexec(). Is the following patch ok for you? --- kernel/panic.c | 8 ++++++++ 1 file changed, 8 insertions(+) --- a/kernel/panic.c +++ b/kernel/panic.c @@ -59,6 +59,7 @@ EXPORT_SYMBOL(panic_blink); */ NORET_TYPE void panic(const char * fmt, ...) { + static DEFINE_SPINLOCK(panic_lock); static char buf[1024]; va_list args; long i, i_next = 0; @@ -82,6 +83,13 @@ NORET_TYPE void panic(const char * fmt, #endif /* + * Only one CPU is allowed to execute the panic code from here. For + * multiple parallel invocations of panic all other CPUs will wait on + * the panic_lock. They are stopped afterwards by smp_send_stop(). + */ + spin_lock(&panic_lock); + + /* * If we have crashed and we have a crash kernel loaded let it handle * everything else. * Do we want to call this before we try to display a message? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-25 14:58 ` Michael Holzheu @ 2011-10-25 15:08 ` Vivek Goyal 2011-10-25 15:28 ` Michael Holzheu 2011-10-25 15:28 ` Don Zickus 0 siblings, 2 replies; 14+ messages in thread From: Vivek Goyal @ 2011-10-25 15:08 UTC (permalink / raw) To: Michael Holzheu Cc: Eric W. Biederman, Américo Wang, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel, Don Zickus On Tue, Oct 25, 2011 at 04:58:19PM +0200, Michael Holzheu wrote: > On Tue, 2011-10-25 at 05:04 -0700, Eric W. Biederman wrote: > > Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: > > > > > Hello Eric, > > > > > > On Mon, 2011-10-24 at 10:07 -0700, Eric W. Biederman wrote: > > > > > > [snip] > > > > > >> So my second thought is to introduce another atomic variable > > >> panic_in_progress, visible only in panic. The cpu that sets > > >> increments panic_in_progress can call smp_send_stop. The rest of > > >> the cpus can just go into a busy wait. That should stop nasty > > >> fights about who is going to come out of smp_send_stop first. > > > > > > So this is a spinlock, no? What about the following patch: > > Do we want both panic printks? > > Ok, good point. We proably should not change that. > > > We really only need the mutual exclusion starting just before > > smp_send_stop so that is where I would be inclined to put it. > > I think to fix the race, at least we have the get the lock before we > call crash_kexec(). > > Is the following patch ok for you? > --- > kernel/panic.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -59,6 +59,7 @@ EXPORT_SYMBOL(panic_blink); > */ > NORET_TYPE void panic(const char * fmt, ...) > { > + static DEFINE_SPINLOCK(panic_lock); > static char buf[1024]; > va_list args; > long i, i_next = 0; > @@ -82,6 +83,13 @@ NORET_TYPE void panic(const char * fmt, > #endif > > /* > + * Only one CPU is allowed to execute the panic code from here. For > + * multiple parallel invocations of panic all other CPUs will wait on > + * the panic_lock. They are stopped afterwards by smp_send_stop(). > + */ > + spin_lock(&panic_lock); Why leave irqs enabled? Atleast for x86, Don Zickus had a patch to use NMI in smp_send_stop(). So that should work even if interrupts are disabled. (I think that patch is not merged yet). So are other architectures a concern? If yes, then may be in future we can make it an arch call which can also choose to disable interrupts. CCing Don also. This lock also brings in the serialization required for panic notifier list and kmsg_dump() infrastructure. Thanks Vivek ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-25 15:08 ` Vivek Goyal @ 2011-10-25 15:28 ` Michael Holzheu 2011-10-25 15:28 ` Don Zickus 1 sibling, 0 replies; 14+ messages in thread From: Michael Holzheu @ 2011-10-25 15:28 UTC (permalink / raw) To: Vivek Goyal Cc: Eric W. Biederman, Américo Wang, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel, Don Zickus On Tue, 2011-10-25 at 11:08 -0400, Vivek Goyal wrote: > On Tue, Oct 25, 2011 at 04:58:19PM +0200, Michael Holzheu wrote: > > On Tue, 2011-10-25 at 05:04 -0700, Eric W. Biederman wrote: > > > Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: [snip] > > > > Is the following patch ok for you? > > --- > > kernel/panic.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > --- a/kernel/panic.c > > +++ b/kernel/panic.c > > @@ -59,6 +59,7 @@ EXPORT_SYMBOL(panic_blink); > > */ > > NORET_TYPE void panic(const char * fmt, ...) > > { > > + static DEFINE_SPINLOCK(panic_lock); > > static char buf[1024]; > > va_list args; > > long i, i_next = 0; > > @@ -82,6 +83,13 @@ NORET_TYPE void panic(const char * fmt, > > #endif > > > > /* > > + * Only one CPU is allowed to execute the panic code from here. For > > + * multiple parallel invocations of panic all other CPUs will wait on > > + * the panic_lock. They are stopped afterwards by smp_send_stop(). > > + */ > > + spin_lock(&panic_lock); > > Why leave irqs enabled? > > Atleast for x86, Don Zickus had a patch to use NMI in smp_send_stop(). So > that should work even if interrupts are disabled. (I think that patch is > not merged yet). > > So are other architectures a concern? If yes, then may be in future we > can make it an arch call which can also choose to disable interrupts. For s390 we could disable the interrupts here. smp_send_stop() works also when IRQs are disabled. But as you said - who knows if that is true on all architectures... Michael ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: kdump: crash_kexec()-smp_send_stop() race in panic 2011-10-25 15:08 ` Vivek Goyal 2011-10-25 15:28 ` Michael Holzheu @ 2011-10-25 15:28 ` Don Zickus 1 sibling, 0 replies; 14+ messages in thread From: Don Zickus @ 2011-10-25 15:28 UTC (permalink / raw) To: Vivek Goyal Cc: Michael Holzheu, Eric W. Biederman, Américo Wang, akpm, schwidefsky, heiko.carstens, kexec, linux-kernel On Tue, Oct 25, 2011 at 11:08:30AM -0400, Vivek Goyal wrote: > On Tue, Oct 25, 2011 at 04:58:19PM +0200, Michael Holzheu wrote: > > On Tue, 2011-10-25 at 05:04 -0700, Eric W. Biederman wrote: > > > Michael Holzheu <holzheu@linux.vnet.ibm.com> writes: > > > > > > > Hello Eric, > > > > > > > > On Mon, 2011-10-24 at 10:07 -0700, Eric W. Biederman wrote: > > > > > > > > [snip] > > > > > > > >> So my second thought is to introduce another atomic variable > > > >> panic_in_progress, visible only in panic. The cpu that sets > > > >> increments panic_in_progress can call smp_send_stop. The rest of > > > >> the cpus can just go into a busy wait. That should stop nasty > > > >> fights about who is going to come out of smp_send_stop first. > > > > > > > > So this is a spinlock, no? What about the following patch: > > > Do we want both panic printks? > > > > Ok, good point. We proably should not change that. > > > > > We really only need the mutual exclusion starting just before > > > smp_send_stop so that is where I would be inclined to put it. > > > > I think to fix the race, at least we have the get the lock before we > > call crash_kexec(). > > > > Is the following patch ok for you? > > --- > > kernel/panic.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > --- a/kernel/panic.c > > +++ b/kernel/panic.c > > @@ -59,6 +59,7 @@ EXPORT_SYMBOL(panic_blink); > > */ > > NORET_TYPE void panic(const char * fmt, ...) > > { > > + static DEFINE_SPINLOCK(panic_lock); > > static char buf[1024]; > > va_list args; > > long i, i_next = 0; > > @@ -82,6 +83,13 @@ NORET_TYPE void panic(const char * fmt, > > #endif > > > > /* > > + * Only one CPU is allowed to execute the panic code from here. For > > + * multiple parallel invocations of panic all other CPUs will wait on > > + * the panic_lock. They are stopped afterwards by smp_send_stop(). > > + */ > > + spin_lock(&panic_lock); > > Why leave irqs enabled? > > Atleast for x86, Don Zickus had a patch to use NMI in smp_send_stop(). So > that should work even if interrupts are disabled. (I think that patch is > not merged yet). > > So are other architectures a concern? If yes, then may be in future we > can make it an arch call which can also choose to disable interrupts. > > CCing Don also. This lock also brings in the serialization required for > panic notifier list and kmsg_dump() infrastructure. This serializes panics, for kmsg_dump we wanted to serialize the shutdown path, IOW stop all the cpus realiably. This patch solves a different problem. Cheers, Don ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-10-25 15:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-24 14:55 kdump: crash_kexec()-smp_send_stop() race in panic Michael Holzheu 2011-10-24 15:14 ` Eric W. Biederman 2011-10-24 15:23 ` Américo Wang 2011-10-24 17:07 ` Eric W. Biederman 2011-10-24 17:33 ` Vivek Goyal 2011-10-24 22:24 ` Seiji Aguchi 2011-10-25 8:33 ` Michael Holzheu 2011-10-25 8:44 ` Michael Holzheu 2011-10-25 12:04 ` Eric W. Biederman 2011-10-25 14:54 ` Vivek Goyal 2011-10-25 14:58 ` Michael Holzheu 2011-10-25 15:08 ` Vivek Goyal 2011-10-25 15:28 ` Michael Holzheu 2011-10-25 15:28 ` Don Zickus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).