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

PATCH In review Pan Xinhui 3 participants 5 messages May 14, 2015 – Jun 30, 2015 Stable branch

lore

Share & tools

lore

Pan Xinhui

author

2015-05-14 09:06 UTC #0 lore raw

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

this patch reverts commit 3440a1 which causes the regression and fix a list corruption.

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 will lost control of pointer cp. cp is passed in and may be freed later in other places.

Signed-off-by Pan Xinhui <[email protected]>

---
Changes in v2:
no codes changed from v1, just update the comment.
upstream commit fc21c0 fix this issue, as it removes the total feature. :) the buggy codes exist in v3.10 and v3.12.

 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

Greg KH

2015-05-14 12:50 UTC #1 lore raw

Quoted 14 lines · Pan Xinhui

this patch reverts commit 3440a1 which causes the regression and fix a list corruption.
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 will lost control of pointer cp. cp is passed in and may be freed later in other places.

Quoted 6 lines

Signed-off-by Pan Xinhui <[email protected]>
---
Changes in v2:
no codes changed from v1, just update the comment.
upstream commit fc21c0 fix this issue, as it removes the total feature. :) the buggy codes exist in v3.10 and v3.12.

Why shouldn't we just include fc21c0 instead? I don't like patches that are not identical to what is in Linus's tree.

thanks,

greg k-h

Pan Xinhui

author

2015-05-14 13:08 UTC #2 lore raw

hi, Greg

Quoted 22 lines · Greg KH

Quoted 14 lines · Pan Xinhui
this patch reverts commit 3440a1 which causes the regression and fix a list corruption.
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 will lost control of pointer cp. cp is passed in and may be freed later in other places.
Quoted 6 lines
Signed-off-by Pan Xinhui <[email protected]>
---
Changes in v2:
no codes changed from v1, just update the comment.
upstream commit fc21c0 fix this issue, as it removes the total feature. :) the buggy codes exist in v3.10 and v3.12.

Quoted 3 lines

Why shouldn't we just include fc21c0 instead? I don't like patches that are not identical to what is in Linus's tree.

yes, I hope to keep same patches with Linus's tree, too. But this feature works well with my patch in :) As far as I know, commit fc21c0 is bigger than 100 lines.

 18  include/linux/interrupt.h |  22 --------
 19  kernel/softirq.c          | 131 ----------------------------------------------

I am a little afraid that will broke something, and someone else using this feature, like me, will complain.

So I want to fix the panic with little codes changed.

thanks for your reply.

thanks.
xinhui

Quoted 4 lines

thanks,
greg k-h

Greg KH

2015-05-15 18:32 UTC #3 lore raw

Quoted 28 lines · Pan Xinhui

hi, Greg
Quoted 22 lines · Greg KH
Quoted 14 lines · Pan Xinhui
this patch reverts commit 3440a1 which causes the regression and fix a list corruption.
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 will lost control of pointer cp. cp is passed in and may be freed later in other places.
Quoted 6 lines
Signed-off-by Pan Xinhui <[email protected]>
---
Changes in v2:
no codes changed from v1, just update the comment.
upstream commit fc21c0 fix this issue, as it removes the total feature. :) the buggy codes exist in v3.10 and v3.12.
Quoted 3 lines
Why shouldn't we just include fc21c0 instead? I don't like patches that are not identical to what is in Linus's tree.

Quoted 6 lines

yes, I hope to keep same patches with Linus's tree, too. But this feature works well with my patch in :) As far as I know, commit fc21c0 is bigger than 100 lines.
 18  include/linux/interrupt.h |  22 --------
 19  kernel/softirq.c          | 131 ----------------------------------------------
I am a little afraid that will broke something, and someone else using this feature, like me, will complain.

I'd rather take this patch that rips it out, as that is what is in Linus's tree, than to take the chance that your "fix" is incorrect.

Quoted 1 line

So I want to fix the panic with little codes changed.

It's safer to be identical than to try to reduce code change, we've learned this the hard way over many years.

thanks,

greg k-h

Jiri Slaby

2015-06-30 12:17 UTC #4 lore raw

In reply to Pan Xinhui

Quoted 20 lines · Pan Xinhui

this patch reverts commit 3440a1 which causes the regression and fix a list corruption.
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 will lost control of pointer cp. cp is passed in and may be freed later in other places.

Quoted 6 lines

Signed-off-by Pan Xinhui <[email protected]>
---
Changes in v2:
no codes changed from v1, just update the comment.
upstream commit fc21c0 fix this issue, as it removes the total feature. :) the buggy codes exist in v3.10 and v3.12.

Applied fc21c0cff2f425891b28ff6fb6b03b325c977428 to 3.12. Thanks.

Signature · 2 lines

js
suse labs