Skip to contentSkip to conversation

kernel/smp.c: fix a panic as cp->info is used wrongly and a list corruption

PATCH Superseded Pan Xinhui 2 participants 3 messages May 12, 2015 – May 14, 2015 Stable branch

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Pan Xinhui

author
#0 lore raw

PATCH 1/1 kernel/smp.c: fix a panic as cp->info is used wrongly and a list corruption

this patch reverts commit 3440a1ca99707f093e9568ba9762764d3162dd8f which causes the regression.

base knowledge: kernel call cp->func using cp->info as its argument. like cp->func(cp->info);

current code is totally wrong, as 1) &softirq is at stack. 2) cp->info don't point to struct call_single_data. So in remote_softirq_receive,
1) If the caller had left __try_remote_softirq, dereferencing cp->info could not fetch the correct value.
2) And we can't get struct call_single_data *cp anymore.

The list corruption is below.
__local_trigger will add cp->list into softirq_work_list. But no one will delete cp->list on behalf of us. if we can succeed to raise_softirq_irqoff, we must delete it from softirq_work_list. because we has lost control of pointer cp. cp is passed in and may be freed later in other places.

Cc <[email protected]> # 3.10
Signed-off-by xinhuix.pan <[email protected]>
---
 include/linux/smp.h |  1 +
 kernel/softirq.c    | 10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)
include/linux/smp.h +1 -0
index c848876..5b790c3 100644
@@ -21,6 +21,7 @@ struct call_single_data {
 	smp_call_func_t func;
 	void *info;
 	u16 flags;
+	u16 priv;
 };
 
 /* total number of cpus in this system (may exceed NR_CPUS) */
kernel/softirq.c +7 -3
index 3d6833f..46308b1 100644
@@ -625,8 +625,11 @@ static void __local_trigger(struct call_single_data *cp, int softirq)
 	list_add_tail(&cp->list, head);
 
 	/* Trigger the softirq only if the list was previously empty.  */
-	if (head->next == &cp->list)
+	if (head->next == &cp->list) {
 		raise_softirq_irqoff(softirq);
+		/*no other places will delete this list_head, we need delete it.*/
+		list_del(&cp->list);
+	}
 }
 
 #ifdef CONFIG_USE_GENERIC_SMP_HELPERS

@@ -636,7 +639,7 @@ static void remote_softirq_receive(void *data)
 	unsigned long flags;
 	int softirq;
 
-	softirq = *(int *)cp->info;
+	softirq = cp->priv;
 	local_irq_save(flags);
 	__local_trigger(cp, softirq);
 	local_irq_restore(flags);

@@ -646,8 +649,9 @@ static int __try_remote_softirq(struct call_single_data *cp, int cpu, int softir
 {
 	if (cpu_online(cpu)) {
 		cp->func = remote_softirq_receive;
-		cp->info = &softirq;
+		cp->info = cp;
 		cp->flags = 0;
+		cp->priv = softirq;
 
 		__smp_call_function_single(cpu, cp, 0);
 		return 0;
Signature · 1 line
1.9.1
#1 lore raw
Quoted 14 lines · Pan Xinhui

this patch reverts commit 3440a1ca99707f093e9568ba9762764d3162dd8f which causes the regression.

base knowledge: kernel call cp->func using cp->info as its argument. like cp->func(cp->info);

current code is totally wrong, as 1) &softirq is at stack. 2) cp->info don't point to struct call_single_data. So in remote_softirq_receive,
1) If the caller had left __try_remote_softirq, dereferencing cp->info could not fetch the correct value.
2) And we can't get struct call_single_data *cp anymore.

The list corruption is below.
__local_trigger will add cp->list into softirq_work_list. But no one will delete cp->list on behalf of us. if we can succeed to raise_softirq_irqoff, we must delete it from softirq_work_list. because we has lost control of pointer cp. cp is passed in and may be freed later in other places.

Quoted 6 lines
Cc <[email protected]> # 3.10
Signed-off-by xinhuix.pan <[email protected]>
---
 include/linux/smp.h |  1 +
 kernel/softirq.c    | 10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.

</formletter>

author
#2 lore raw

hi,Greg
thanks for your kind reply :)
I will send out patch V2 later. let me do some explanation here first.
this feature which I am working on is reverted totally by commit fc21c0 in upstream, maybe the author notice the code is buggy. So I just fix it, not revert this feature here, big change is not accepted in stable kernel tree.

thanks again for your help.

thanks
xinhui

Quoted 30 lines · Greg KH
Quoted 14 lines · Pan Xinhui

this patch reverts commit 3440a1ca99707f093e9568ba9762764d3162dd8f which causes the regression.

base knowledge: kernel call cp->func using cp->info as its argument. like cp->func(cp->info);

current code is totally wrong, as 1) &softirq is at stack. 2) cp->info don't point to struct call_single_data. So in remote_softirq_receive,
1) If the caller had left __try_remote_softirq, dereferencing cp->info could not fetch the correct value.
2) And we can't get struct call_single_data *cp anymore.

The list corruption is below.
__local_trigger will add cp->list into softirq_work_list. But no one will delete cp->list on behalf of us. if we can succeed to raise_softirq_irqoff, we must delete it from softirq_work_list. because we has lost control of pointer cp. cp is passed in and may be freed later in other places.

Quoted 6 lines
Cc <[email protected]> # 3.10
Signed-off-by xinhuix.pan <[email protected]>
---
  include/linux/smp.h |  1 +
  kernel/softirq.c    | 10 +++++++---
  2 files changed, 8 insertions(+), 3 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.

</formletter>