[PATCH] Fix kexec abort due to IPI from panic().

[PATCH] Fix kexec abort due to IPI from panic().

[Seiji Aguchi]

Hi,

I'm Seiji Aguchi.
I work for Hitachi Data Systems.
It's a first time to send a patch to lkml.
Nice to meet you.
 
I found an issue in kexec.
Please give me your comments and suggestions.
 
Kexec abort when two cpus panic at the same time.
An example scenario:
1. Two cpus panic at the same time .
2. One cpu ,cpu0, get kexec_mutex in crash_kexec().
3. The other cpu ,cpu1, can't get kexec_mutex and return from crash_kexec().
4. Cpu0 runs kmsg_dump(KMSG_DUMP_KEXEC).
5. Cpu1 can't get dump_list_lock and return from kmsg_dump(KMSG_DUMP_PANIC).
6. Cpu1 runs smp_send_stop() in panic() and sends IPI to other cpus.
7. Cpu0 may receive IPI from cpu1 while running kmsg_dump(KMSG_DUMP_KEXEC),
   crash_setup_regs(), or crash_save_vmcore().
 
We can solve this issue by disabling external interrupt while getting kexec_mutex 
in crash_kexec().


 Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>

---
 kernel/kexec.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index c0613f7..9e9f159 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1075,6 +1075,10 @@ void crash_kexec(struct pt_regs *regs)
 	 * sufficient.  But since I reuse the memory...
 	 */
 	if (mutex_trylock(&kexec_mutex)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+
 		if (kexec_crash_image) {
 			struct pt_regs fixed_regs;
 
@@ -1085,6 +1089,9 @@ void crash_kexec(struct pt_regs *regs)
 			machine_crash_shutdown(&fixed_regs);
 			machine_kexec(kexec_crash_image);
 		}
+
+		local_irq_restore(flags);
+
 		mutex_unlock(&kexec_mutex);
 	}
 }
-- 
1.7.2.2


Regards,

Seiji

Re: [PATCH] Fix kexec abort due to IPI from panic().

[Eric W. Biederman]

Seiji Aguchi <seiji.aguchi@hds.com> writes:

> Hi,
>
> I'm Seiji Aguchi.
> I work for Hitachi Data Systems.
> It's a first time to send a patch to lkml.
> Nice to meet you.
>  
> I found an issue in kexec.
> Please give me your comments and suggestions.
>  
> Kexec abort when two cpus panic at the same time.
> An example scenario:
> 1. Two cpus panic at the same time .
> 2. One cpu ,cpu0, get kexec_mutex in crash_kexec().
> 3. The other cpu ,cpu1, can't get kexec_mutex and return from crash_kexec().
> 4. Cpu0 runs kmsg_dump(KMSG_DUMP_KEXEC).
> 5. Cpu1 can't get dump_list_lock and return from kmsg_dump(KMSG_DUMP_PANIC).
> 6. Cpu1 runs smp_send_stop() in panic() and sends IPI to other cpus.
> 7. Cpu0 may receive IPI from cpu1 while running kmsg_dump(KMSG_DUMP_KEXEC),
>    crash_setup_regs(), or crash_save_vmcore().
>  
> We can solve this issue by disabling external interrupt while getting kexec_mutex 
> in crash_kexec().

Disabling interrupts is fine, I thought we did that already at some
point.  However that call to kmsg_dump(KMSG_DUMP_KEXEC) is a bug as it
introduces locks into a path that should not be taking locks.  Please
remove that broken kmsg_dump call as well.

Nothing in the crash_kexec path should even have the option of blocking.

Eric

RE: [PATCH] Fix kexec abort due to IPI from panic().

[Seiji Aguchi]

Hi Eric,

Thank you for your quick response!

>Disabling interrupts is fine,

Thanks!

>However that call to kmsg_dump(KMSG_DUMP_KEXEC) is a bug as it introduces locks 
>into a path that should not be taking locks.

I'd like to find a way that kexec coexists with kmsg_dump(KMSG_DUMP_KEXEC) because 
kmsg_dump is a useful troubleshooting feature as well.

So, I will improve kmsg_dump(KMSG_DUMP_KEXEC) if there are some bugs.
Could you please let me know your concern?
It is helpful for me if you have an example scenario kexec fails.

>Nothing in the crash_kexec path should even have the option of blocking.

Do you mean I need to change kmsg_dump(KMSG_DUMP_KEXEC) to lockless?

Seiji

[RFC][PATCH] Fix kexec abort due to IPI from panic().

[Seiji Aguchi]

Hi Eric,

This is a patch which makes kmsg_dump() non-blocking.
Please give me your comments and suggestions.

I improved it as follows.

(1) Improvement of dump_list_lock
    (1-1) I changed dump_list to RCU for deleting dump_list_lock in kmsg_dump().
    (1-2) I moved kmsg_dump(KMSG_DUMP_KEXEC) behind machine_crash_shutdown() 
          for avoiding concurrent execution of dump_list functions.
    (1-3) I also moved kmsg_dump(KMSG_DUMP_PANIC) behind smp_send_stop() for the 
          same reason.
 
(2) Improvement of logbuf_lock
    I added spinlock_init(&logbuf_lock) when executing kmsg_dump() in kexec or panic path
    for preventing dead lock.

We can delete blocking kmsg_dump call in crash_kexec and panic path.

 Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>

---
 kernel/kexec.c  |   10 ++++++++--
 kernel/panic.c  |    4 ++--
 kernel/printk.c |   19 ++++++++++---------
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index c0613f7..fdc6bfc 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1075,16 +1075,22 @@ void crash_kexec(struct pt_regs *regs)
 	 * sufficient.  But since I reuse the memory...
 	 */
 	if (mutex_trylock(&kexec_mutex)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+
 		if (kexec_crash_image) {
 			struct pt_regs fixed_regs;
 
-			kmsg_dump(KMSG_DUMP_KEXEC);
-
 			crash_setup_regs(&fixed_regs, regs);
 			crash_save_vmcoreinfo();
 			machine_crash_shutdown(&fixed_regs);
+			kmsg_dump(KMSG_DUMP_KEXEC);
 			machine_kexec(kexec_crash_image);
 		}
+
+		local_irq_restore(flags);
+
 		mutex_unlock(&kexec_mutex);
 	}
 }
diff --git a/kernel/panic.c b/kernel/panic.c
index 4c13b1a..e14d1d6 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -87,8 +87,6 @@ NORET_TYPE void panic(const char * fmt, ...)
 	 */
 	crash_kexec(NULL);
 
-	kmsg_dump(KMSG_DUMP_PANIC);
-
 	/*
 	 * Note smp_send_stop is the usual smp shutdown function, which
 	 * unfortunately means it may not be hardened to work in a panic
@@ -96,6 +94,8 @@ NORET_TYPE void panic(const char * fmt, ...)
 	 */
 	smp_send_stop();
 
+	kmsg_dump(KMSG_DUMP_PANIC);
+
 	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
 
 	bust_spinlocks(0);
diff --git a/kernel/printk.c b/kernel/printk.c
index 8fe465a..6d6b09f 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1478,7 +1478,7 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
 	/* Don't allow registering multiple times */
 	if (!dumper->registered) {
 		dumper->registered = 1;
-		list_add_tail(&dumper->list, &dump_list);
+		list_add_tail_rcu(&dumper->list, &dump_list);
 		err = 0;
 	}
 	spin_unlock_irqrestore(&dump_list_lock, flags);
@@ -1502,10 +1502,11 @@ int kmsg_dump_unregister(struct kmsg_dumper *dumper)
 	spin_lock_irqsave(&dump_list_lock, flags);
 	if (dumper->registered) {
 		dumper->registered = 0;
-		list_del(&dumper->list);
+		list_del_rcu(&dumper->list);
 		err = 0;
 	}
 	spin_unlock_irqrestore(&dump_list_lock, flags);
+	synchronize_rcu();
 
 	return err;
 }
@@ -1541,6 +1542,10 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 	unsigned long l1, l2;
 	unsigned long flags;
 
+
+	if (reason == KMSG_DUMP_KEXEC || reason == KMSG_DUMP_PANIC)
+		spin_lock_init(&logbuf_lock);
+
 	/* Theoretically, the log could move on after we do this, but
 	   there's not a lot we can do about that. The new messages
 	   will overwrite the start of what we dump. */
@@ -1563,13 +1568,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 		l2 = chars;
 	}
 
-	if (!spin_trylock_irqsave(&dump_list_lock, flags)) {
-		printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
-				kmsg_to_str(reason));
-		return;
-	}
-	list_for_each_entry(dumper, &dump_list, list)
+	rcu_read_lock();
+	list_for_each_entry_rcu(dumper, &dump_list, list)
 		dumper->dump(dumper, reason, s1, l1, s2, l2);
-	spin_unlock_irqrestore(&dump_list_lock, flags);
+	rcu_read_unlock();
 }
 #endif
-- 
1.7.2.2


Seiji

Re: [RFC][PATCH] Fix kexec abort due to IPI from panic().

[Eric W. Biederman]

Seiji Aguchi <seiji.aguchi@hds.com> writes:

> Hi Eric,
>
> This is a patch which makes kmsg_dump() non-blocking.
> Please give me your comments and suggestions.
>
> I improved it as follows.
>
> (1) Improvement of dump_list_lock
>     (1-1) I changed dump_list to RCU for deleting dump_list_lock in kmsg_dump().
>     (1-2) I moved kmsg_dump(KMSG_DUMP_KEXEC) behind machine_crash_shutdown() 
>           for avoiding concurrent execution of dump_list functions.
>     (1-3) I also moved kmsg_dump(KMSG_DUMP_PANIC) behind smp_send_stop() for the 
>           same reason.
>  
> (2) Improvement of logbuf_lock
>     I added spinlock_init(&logbuf_lock) when executing kmsg_dump() in kexec or panic path
>     for preventing dead lock.
>
> We can delete blocking kmsg_dump call in crash_kexec and panic path.

This looks better, but it still gives me the willies.

I tried tracing through the ramoops code to see if there were anything
else that could block, but I couldn't make it through do_gettimeofday.

I couldn't even make it that far with the mtd oops tracer.

The fact that the code is exported and modular doesn't make me feel
safe because there have been people in the past who have asked for an
notifier on crash so they could do stupid things when the kernel is
broken.

The fact that this wasn't noticed until we actually had a hang, doesn't
give me an especially great feeling about long term stability.

Most of all I don't see the use case of calling kmsg_dump when you have
kexec on panic setup to do the same thing.  Having kmsg_dump not on
the kexec on panic code path would let me sleep much easier at night.

Then there is the historical side of this.  Through many failed attempts
it has been show that dumpers in the kernel are fragile beasts that work
up until you actually have a real world failure and then they let you
down.  Kexec on panic is better as it works 65% or so of the time,
and definitely won't corrupt your bits if it fails.  I don't see what
makes kmsg_dump better than all of the past failed and useless kernel
dumpers.

Eric

RE: [RFC][PATCH] Fix kexec abort due to IPI from panic().

[Seiji Aguchi]

Hi Eric

Thank you for your detailed explanation.
I understood historic backdrop of kexec in upstream kernel.
I also understood the reason why you are worried about 
kmsg_dump in kexec path.

The problem is that kmsg_dumper or notifier calls are used by people 
who are unfamiliar with kexec because they use blocking-functions 
such as do_gettimeofday unconsciously.

And I still believe that kmsg_dumper in kexec path is an useful 
A troubleshooting feature.

When kexec on panic fails, it is difficult to investigate the reason 
why kexec fails. It comes from lack of materials for the investigation.

Kernel buffer is helpful for the investigation.
So, I would like to continue the discussion about a way of getting 
kernel buffer in kexec path.

I'll participate in End User Summit and Linux Plumbers Conference.
I'd like to meet with you if you join these conferences. 

Seiji