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

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

[Greg KH]

On Wed, May 13, 2015 at 05:03:04PM +0800, Pan Xinhui wrote:
> 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: <stable@vger.kernel.org> # 3.10
> Signed-off-by: xinhuix.pan <xinhuix.pan@intel.com>
> ---
>  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>

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

[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.

Cc: <stable@vger.kernel.org> # 3.10
Signed-off-by: xinhuix.pan <xinhuix.pan@intel.com>
---
  include/linux/smp.h |  1 +
  kernel/softirq.c    | 10 +++++++---
  2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index c848876..5b790c3 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -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) */
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 3d6833f..46308b1 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -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;
-- 
1.9.1

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

[Pan Xinhui]

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

On 2015年05月12日 20:14, Greg KH wrote:
> On Wed, May 13, 2015 at 05:03:04PM +0800, Pan Xinhui wrote:
>> 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: <stable@vger.kernel.org> # 3.10
>> Signed-off-by: xinhuix.pan <xinhuix.pan@intel.com>
>> ---
>>   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>
>