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

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

[Greg KH]

On Fri, May 15, 2015 at 05:04:03PM +0800, Pan Xinhui wrote:
> 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 <xinhuix.pan@intel.com>
> ---
> 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

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

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

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
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(-)

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 V2] kernel/smp.c: fix a panic as cp->info is used wrongly and a, list corruption

[Pan Xinhui]

hi, Greg

On 2015年05月14日 20:50, Greg KH wrote:
> On Fri, May 15, 2015 at 05:04:03PM +0800, Pan Xinhui wrote:
>> 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 <xinhuix.pan@intel.com>
>> ---
>> 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.
>
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

> thanks,
>
> greg k-h
>

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

[Greg KH]

On Fri, May 15, 2015 at 09:07:12PM +0800, Pan Xinhui wrote:
> hi, Greg
> 
> On 2015年05月14日 20:50, Greg KH wrote:
> >On Fri, May 15, 2015 at 05:04:03PM +0800, Pan Xinhui wrote:
> >>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 <xinhuix.pan@intel.com>
> >>---
> >>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.
> >
> 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.

> 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

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

[Jiri Slaby]

On 05/15/2015, 11:04 AM, Pan Xinhui wrote:
> 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 <xinhuix.pan@intel.com>
> ---
> 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.

-- 
js
suse labs