BUG report about ipt_do_table( )

BUG report about ipt_do_table( )

[Wang, Yalin]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

Dear all, 

We encounter a crash in ipt_do_table( ) function 
During our stability test .

The CPU is  qcom msm8960 / dual core  , linux kernel version is 3.4
I have add the two CPU's stack backtrace 
And the crash log .

CPU0 panic( )  in  ipt_do_table( )

Because it get a NULL jumpstack (ipt_entry * *) .

I don't know why it get this NULL value ,
Because the private (xt_table_info *) -> jumpstack[0]  is not NULL .

We have several crashes like this ,
So I think there must be some BUGs here , not hardware issue .

Every time , the panic happened , CPU1 is doing do_replace to replace the 
xt_table_info  info ,  so maybe this is caused by multicore data sync issue ?

CPU0 will see the  old xt_table_info   or the new one .

I am not very clear about ipt_do_table( ) work flow ,
Does  anyone can get some clues from the logs ?


Note:  I am not in linux mail-list now, hope you can add my mail-address
In your cc-loop when you reply to me , Thank you !


Thanks 



Yalin.Wang
Software Engineer 
OS Kernel&Graphics
 
Sony Mobile Communications
Tel: +86 10 5966 9819
Phone: 18610323092
Address: No.16 Guangshun South Street, Chaoyang, Beijing, P.R.C.

sonymobile.com
  
 




[-- Attachment #2: report.zip --]
[-- Type: application/x-zip-compressed, Size: 25346 bytes --]

Re: BUG report about ipt_do_table( )

[Will Deacon]

On Thu, Oct 10, 2013 at 06:16:05AM +0100, Wang, Yalin wrote:
> Dear all, 

Hello,

> We encounter a crash in ipt_do_table( ) function 
> During our stability test .
> 
> The CPU is  qcom msm8960 / dual core  , linux kernel version is 3.4

I appreciate that this is a mammoth task, but can you reproduce this failure
with a mainline kernel (3.12-rc4)? If you suspect a synchronisation issue in
core code, I'm afraid you'll have to show the failure with the current
sources.

Will

RE: BUG report about ipt_do_table( )

[Wang, Yalin]

Hi   Will,

Thanks for your reply .

This is the kernel that we use:

https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/net/ipv4/netfilter/ip_tables.c?id=M8960AAAAANLYA26144005
https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/net/netfilter/x_tables.c?id=M8960AAAAANLYA26144005



I am sorry that I can't use the latest kernel to reproduce
This BUG, make the new kernel running on our 
Platform is not easy, and need do a lot of porting work .

I have compare our kernel with 3.12 ,  
Ip_tables.c x_tables.c  is the same ,
So the BUG should can also be reproduce on 3.12 (just my guess).


I make a patch for this BUG, but have not test it:


/-----------------------------------------------------------------------/
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 8d987c3..2353bcc 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -819,6 +819,12 @@ xt_replace_table(struct xt_table *table,
 		return NULL;
 	}
 
+	/*
+	 * make sure the change is write to the memory
+	 * so that the other CPU can see the changes
+	 */
+	mb();
+
 	/* Do the substitution. */
 	local_bh_disable();
 	private = table->private;

/-----------------------------------------------------------------------/


I add a memory barrier before update table->private .
Make sure the other CPU can see the update memory correctly.
When the BUG happened, the other CPU can get the new private (struct xt_table_info *),
But sometimes it see private->jumpstack == NULL  , or sometimes it see private->jumpstack[cpu] == NULL ,

Our several crash dumps show different crash point .

This is caused by CPU write buffer ? 
It has written table->private , but has not update private-> members (still in write buffer)  ,
This is really out of order write, will this happened on modern armv7 CPU?
Especially like cortex-a15 , it can execute code out of order .


Thanks you .


-----Original Message-----
From: Will Deacon [mailto:will.deacon@arm.com] 
Sent: Thursday, October 10, 2013 5:48 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-owner@vger.kernel.org'; linux-kernel@vger.kernel.org
Subject: Re: BUG report about ipt_do_table( )

On Thu, Oct 10, 2013 at 06:16:05AM +0100, Wang, Yalin wrote:
> Dear all,

Hello,

> We encounter a crash in ipt_do_table( ) function During our stability 
> test .
> 
> The CPU is  qcom msm8960 / dual core  , linux kernel version is 3.4

I appreciate that this is a mammoth task, but can you reproduce this failure with a mainline kernel (3.12-rc4)? If you suspect a synchronisation issue in core code, I'm afraid you'll have to show the failure with the current sources.

Will

Re: BUG report about ipt_do_table( )

[Will Deacon]

On Thu, Oct 10, 2013 at 11:22:21AM +0100, Wang, Yalin wrote:
> Thanks for your reply .

No problem.

> I have compare our kernel with 3.12 ,  
> Ip_tables.c x_tables.c  is the same ,
> So the BUG should can also be reproduce on 3.12 (just my guess).

[...]

> /-----------------------------------------------------------------------/
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 8d987c3..2353bcc 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -819,6 +819,12 @@ xt_replace_table(struct xt_table *table,
>  		return NULL;
>  	}
>  
> +	/*
> +	 * make sure the change is write to the memory
> +	 * so that the other CPU can see the changes
> +	 */
> +	mb();
> +
>  	/* Do the substitution. */
>  	local_bh_disable();
>  	private = table->private;
> 
> /-----------------------------------------------------------------------/
> 
> 
> I add a memory barrier before update table->private .
> Make sure the other CPU can see the update memory correctly.
> When the BUG happened, the other CPU can get the new private (struct xt_table_info *),
> But sometimes it see private->jumpstack == NULL  , or sometimes it see private->jumpstack[cpu] == NULL ,

On one CPU, xt_replace_table is basically doing:

	newinfo->jumpstack = kzalloc(...);
	table->private = newinfo;

so this can be thought of as the `writer' thread.

Then, on another CPU (the `reader'), we run ipt_do_table:

	private = table->private;
	jumpstack  = (struct ipt_entry **)private->jumpstack[cpu];

The reader has an address dependency, so the loads are guaranteed to be
observed in order (on sane CPUs... this is probably broken for Alpha).

However, the two stores from the writer can be observed in any order by other
CPUs. To make this clearer, I think you actually want an smb_wmb() immediately
before the assignment to table->private (and that assignment to
newinfo->initial_entries probably needs moving above it). Then you want an
smb_read_barrier_depends on the read path immediately after reading
table->private.

> This is caused by CPU write buffer ? 
> It has written table->private , but has not update private-> members (still in write buffer)  ,
> This is really out of order write, will this happened on modern armv7 CPU?
> Especially like cortex-a15 , it can execute code out of order .

Well, stores aren't speculated and stores to the same location are always
observed in order (on ARM). This is more a consequence of the weakly ordered
memory model, which largely comes about due to speculative loads and write
buffering (including read forwarding). Things like cache coherence protocols
and buffers in the interconnect can also cause stores to be observed in
different orders, since there conceptually end up being multi copies of the
data being written.

Anyway, can you see if the patch below fixes your problem please?

Will

--->8

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index d23118d..cadda40 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -326,6 +326,7 @@ ipt_do_table(struct sk_buff *skb,
        local_bh_disable();
        addend = xt_write_recseq_begin();
        private = table->private;
+       smp_read_barrier_depends();
        cpu        = smp_processor_id();
        table_base = private->entries[cpu];
        jumpstack  = (struct ipt_entry **)private->jumpstack[cpu];
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 8b03028..ee5e184 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -845,8 +845,9 @@ xt_replace_table(struct xt_table *table,
                return NULL;
        }
 
-       table->private = newinfo;
        newinfo->initial_entries = private->initial_entries;
+       smb_wmb();
+       table->private = newinfo;
 
        /*
         * Even though table entries have now been swapped, other CPU's

RE: BUG report about ipt_do_table( )

[Wang, Yalin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5291 bytes --]

Hi  Will ,

Seems your patch is better than mine,
It make sure newinfo->initial_entries  update is 
Seen by others . 

I will test by your patches , and update to you ASAP .


I have another questions about this  BUG,
Since all shared data will have this problem especially 
Shared between different CPUs,

This means all shared data need a smp_wmb() , after 
One CPU update it ? 
But I see not all shared data are updated by this way,
And kernel works well .  Why ??  a little curious .


Another is that , I read cortex-a15 whitepaper ,
It says :

Load/Store cluster
 All Load/Store, data transfers and cache maintenance operations
 Partially out-of-order, 1 Load and 1 Store executed per cycle
 Load cannot bypass a Store, Store cannot bypass a Store


!! Store can't   bypass  store !! , but in this BUG , it seems later store can
Be wrote into cache even before  the earlier store completed !
Am I right ?


Thank you !

-----Original Message-----
From: Will Deacon [mailto:will.deacon@arm.com] 
Sent: Thursday, October 10, 2013 7:03 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-owner@vger.kernel.org'; linux-kernel@vger.kernel.org; Peng, Arthur; Zhang, Bojie
Subject: Re: BUG report about ipt_do_table( )

On Thu, Oct 10, 2013 at 11:22:21AM +0100, Wang, Yalin wrote:
> Thanks for your reply .

No problem.

> I have compare our kernel with 3.12 , Ip_tables.c x_tables.c  is the 
> same , So the BUG should can also be reproduce on 3.12 (just my 
> guess).

[...]

> /---------------------------------------------------------------------
> --/ diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c 
> index 8d987c3..2353bcc 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -819,6 +819,12 @@ xt_replace_table(struct xt_table *table,
>  		return NULL;
>  	}
>  
> +	/*
> +	 * make sure the change is write to the memory
> +	 * so that the other CPU can see the changes
> +	 */
> +	mb();
> +
>  	/* Do the substitution. */
>  	local_bh_disable();
>  	private = table->private;
> 
> /---------------------------------------------------------------------
> --/
> 
> 
> I add a memory barrier before update table->private .
> Make sure the other CPU can see the update memory correctly.
> When the BUG happened, the other CPU can get the new private (struct 
> xt_table_info *), But sometimes it see private->jumpstack == NULL  , 
> or sometimes it see private->jumpstack[cpu] == NULL ,

On one CPU, xt_replace_table is basically doing:

	newinfo->jumpstack = kzalloc(...);
	table->private = newinfo;

so this can be thought of as the `writer' thread.

Then, on another CPU (the `reader'), we run ipt_do_table:

	private = table->private;
	jumpstack  = (struct ipt_entry **)private->jumpstack[cpu];

The reader has an address dependency, so the loads are guaranteed to be observed in order (on sane CPUs... this is probably broken for Alpha).

However, the two stores from the writer can be observed in any order by other CPUs. To make this clearer, I think you actually want an smb_wmb() immediately before the assignment to table->private (and that assignment to
newinfo->initial_entries probably needs moving above it). Then you want 
newinfo->an
smb_read_barrier_depends on the read path immediately after reading
table->private.

> This is caused by CPU write buffer ? 
> It has written table->private , but has not update private-> members 
> (still in write buffer)  , This is really out of order write, will this happened on modern armv7 CPU?
> Especially like cortex-a15 , it can execute code out of order .

Well, stores aren't speculated and stores to the same location are always observed in order (on ARM). This is more a consequence of the weakly ordered memory model, which largely comes about due to speculative loads and write buffering (including read forwarding). Things like cache coherence protocols and buffers in the interconnect can also cause stores to be observed in different orders, since there conceptually end up being multi copies of the data being written.

Anyway, can you see if the patch below fixes your problem please?

Will

--->8

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index d23118d..cadda40 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -326,6 +326,7 @@ ipt_do_table(struct sk_buff *skb,
        local_bh_disable();
        addend = xt_write_recseq_begin();
        private = table->private;
+       smp_read_barrier_depends();
        cpu        = smp_processor_id();
        table_base = private->entries[cpu];
        jumpstack  = (struct ipt_entry **)private->jumpstack[cpu]; diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c index 8b03028..ee5e184 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -845,8 +845,9 @@ xt_replace_table(struct xt_table *table,
                return NULL;
        }
 
-       table->private = newinfo;
        newinfo->initial_entries = private->initial_entries;
+       smb_wmb();
+       table->private = newinfo;
 
        /*
         * Even though table entries have now been swapped, other CPU's

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: BUG report about ipt_do_table( )

[Will Deacon]

On Thu, Oct 10, 2013 at 12:26:38PM +0100, Wang, Yalin wrote:
> Hi  Will ,

Hello,

> Seems your patch is better than mine,
> It make sure newinfo->initial_entries  update is 
> Seen by others . 
> 
> I will test by your patches , and update to you ASAP .

Thanks.

> I have another questions about this  BUG,
> Since all shared data will have this problem especially 
> Shared between different CPUs,
> 
> This means all shared data need a smp_wmb() , after 
> One CPU update it ? 

Only if the ordering of observability matters and isn't enforced by other
mechanisms (e.g. locks).

> But I see not all shared data are updated by this way,
> And kernel works well .  Why ??  a little curious .

Again, the barriers tend to be hidden inside things like locking primitives,
atomics, I/O accessors etc.

> Another is that , I read cortex-a15 whitepaper ,
> It says :
> 
> Load/Store cluster
>  All Load/Store, data transfers and cache maintenance operations
>  Partially out-of-order, 1 Load and 1 Store executed per cycle
>  Load cannot bypass a Store, Store cannot bypass a Store

It's hard to interpret this out of context. Please can you point me at the
whitepaper? Also, I was talking from the perspective of the architecture and
you're not using an A15 afaik.

> !! Store can't   bypass  store !! , but in this BUG , it seems later store can
> Be wrote into cache even before  the earlier store completed !
> Am I right ?

The architecture requires stores to be observed in program order from the
perspective of the CPU issuing them, but there aren't ordering guarantees
for stores to different locations when observed by a different CPU.

Will

RE: BUG report about ipt_do_table( )

[Wang, Yalin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3260 bytes --]

Hi  Will,

Maybe I know your meaning ,
If it use spinlock to protected the shared data,
The bug will not happen, because spinlock will 
Use DSB( )  to sync .
Unluckily, here, it use a special seqcount_t( ) (see get_counters( ) function)
To make sure there is no others using the old data,
Before release the old data, this is much like RCU
Work, but RCU use rcu_assign_pointer( ) -->
Which use smp_wmb( ) , so it's safe,  am I right ?

In my patch, I use mb( ), because this macro
Is DSB( ) , while smp_wmb( ) is DMS( ),
I just think DSB is much strict than DMS,
mmm..  so , DSM( )  or DMS ( )  are both ok ?



The whitepaper I use is here:
https://www.google.com/#q=cortex+a15+microarchitecture

the first: [PDF] Exploring the Design of the Cortex-A15 Processor - ARM

I just search in Google, and you know that qcom don't release
Much document about its krait cpu's micro architecture details,
I just use cortex-a15 for a reference, I am not sure if their 
pipeline ( load/store unit) are the same,
if you have cortex-a15 micro architecture details document,
I am very appreciated that you can share to  me . :)

Thanks 




-----Original Message-----
From: Will Deacon [mailto:will.deacon@arm.com] 
Sent: Thursday, October 10, 2013 10:19 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-owner@vger.kernel.org'; linux-kernel@vger.kernel.org; Peng, Arthur; Zhang, Bojie; Gu, Youcai 1 (EXT); Alevoor, Raghavendra 2
Subject: Re: BUG report about ipt_do_table( )

On Thu, Oct 10, 2013 at 12:26:38PM +0100, Wang, Yalin wrote:
> Hi  Will ,

Hello,

> Seems your patch is better than mine,
> It make sure newinfo->initial_entries  update is Seen by others .
> 
> I will test by your patches , and update to you ASAP .

Thanks.

> I have another questions about this  BUG, Since all shared data will 
> have this problem especially Shared between different CPUs,
> 
> This means all shared data need a smp_wmb() , after One CPU update it 
> ?

Only if the ordering of observability matters and isn't enforced by other mechanisms (e.g. locks).

> But I see not all shared data are updated by this way, And kernel 
> works well .  Why ??  a little curious .

Again, the barriers tend to be hidden inside things like locking primitives, atomics, I/O accessors etc.

> Another is that , I read cortex-a15 whitepaper , It says :
> 
> Load/Store cluster
> § All Load/Store, data transfers and cache maintenance operations § 
> Partially out-of-order, 1 Load and 1 Store executed per cycle § Load 
> cannot bypass a Store, Store cannot bypass a Store

It's hard to interpret this out of context. Please can you point me at the whitepaper? Also, I was talking from the perspective of the architecture and you're not using an A15 afaik.

> !! Store can't   bypass  store !! , but in this BUG , it seems later store can
> Be wrote into cache even before  the earlier store completed !
> Am I right ?

The architecture requires stores to be observed in program order from the perspective of the CPU issuing them, but there aren't ordering guarantees for stores to different locations when observed by a different CPU.

Will
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: BUG report about ipt_do_table( )

[Will Deacon]

On Fri, Oct 11, 2013 at 02:50:24AM +0100, Wang, Yalin wrote:
> Hi  Will,

Hello again,

> Maybe I know your meaning ,
> If it use spinlock to protected the shared data,
> The bug will not happen, because spinlock will 
> Use DSB( )  to sync .

Actually, the dsb is for something else (the sev). It is the smp_mb() call
which guarantees the ordering of critical sections with respect to spinlock
operations.

> Unluckily, here, it use a special seqcount_t( ) (see get_counters( ) function)

Well, there is a comment about a write_lock being held, so you should be ok
if that's true. The issue I saw was with the newinfo population, as I
described in my earlier mail.

> To make sure there is no others using the old data,
> Before release the old data, this is much like RCU
> Work, but RCU use rcu_assign_pointer( ) -->
> Which use smp_wmb( ) , so it's safe,  am I right ?

RCU is safe. There are *many* weakly ordered architectures on which Linux
runs, so I don't think you have to worry too much about the core data
structures and locking/synchronisation/atomic primitives. The major scope
for errors is in lockless code, where the barrier usage is explicit.

> In my patch, I use mb( ), because this macro
> Is DSB( ) , while smp_wmb( ) is DMS( ),
> I just think DSB is much strict than DMS,
> mmm..  so , DSM( )  or DMS ( )  are both ok ?

I think you're getting confused with your barriers. We have two memory
barriers on ARM: dmb and dsb. dmb is sufficient to enforce ordering of
observability. dsb is used to enforce completion.

> The whitepaper I use is here:
> https://www.google.com/#q=cortex+a15+microarchitecture
> 
> the first: [PDF] Exploring the Design of the Cortex-A15 Processor - ARM
> 
> I just search in Google, and you know that qcom don't release
> Much document about its krait cpu's micro architecture details,
> I just use cortex-a15 for a reference, I am not sure if their 
> pipeline ( load/store unit) are the same,

I think the lawyers would have a field day if the pipelines were the same!
You really can't use an A15 slide-deck to infer micro-architectural details
about Krait.

Please can you test the patch I sent you yesterday?

Will

RE: BUG report about ipt_do_table( )

[Wang, Yalin]

Hi Will,

Thanks for your clarification .

I have merged the patch,
And need wait some time to get
The test result ,
You know that this BUG is not easy to reproduce,
It's very infrequent.
Maybe we need run several times stability test to 
Make sure the patch works .

I will update to you the result as soon as possible !

Thanks 

-----Original Message-----
From: Will Deacon [mailto:will.deacon@arm.com] 
Sent: Friday, October 11, 2013 7:03 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-owner@vger.kernel.org'; linux-kernel@vger.kernel.org; Peng, Arthur; Zhang, Bojie; Gu, Youcai 1 (EXT); Alevoor, Raghavendra 2
Subject: Re: BUG report about ipt_do_table( )

On Fri, Oct 11, 2013 at 02:50:24AM +0100, Wang, Yalin wrote:
> Hi  Will,

Hello again,

> Maybe I know your meaning ,
> If it use spinlock to protected the shared data, The bug will not 
> happen, because spinlock will Use DSB( )  to sync .

Actually, the dsb is for something else (the sev). It is the smp_mb() call which guarantees the ordering of critical sections with respect to spinlock operations.

> Unluckily, here, it use a special seqcount_t( ) (see get_counters( ) 
> function)

Well, there is a comment about a write_lock being held, so you should be ok if that's true. The issue I saw was with the newinfo population, as I described in my earlier mail.

> To make sure there is no others using the old data, Before release the 
> old data, this is much like RCU Work, but RCU use rcu_assign_pointer( 
> ) --> Which use smp_wmb( ) , so it's safe,  am I right ?

RCU is safe. There are *many* weakly ordered architectures on which Linux runs, so I don't think you have to worry too much about the core data structures and locking/synchronisation/atomic primitives. The major scope for errors is in lockless code, where the barrier usage is explicit.

> In my patch, I use mb( ), because this macro Is DSB( ) , while 
> smp_wmb( ) is DMS( ), I just think DSB is much strict than DMS, mmm..  
> so , DSM( )  or DMS ( )  are both ok ?

I think you're getting confused with your barriers. We have two memory barriers on ARM: dmb and dsb. dmb is sufficient to enforce ordering of observability. dsb is used to enforce completion.

> The whitepaper I use is here:
> https://www.google.com/#q=cortex+a15+microarchitecture
> 
> the first: [PDF] Exploring the Design of the Cortex-A15 Processor - 
> ARM
> 
> I just search in Google, and you know that qcom don't release Much 
> document about its krait cpu's micro architecture details, I just use 
> cortex-a15 for a reference, I am not sure if their pipeline ( 
> load/store unit) are the same,

I think the lawyers would have a field day if the pipelines were the same!
You really can't use an A15 slide-deck to infer micro-architectural details about Krait.

Please can you test the patch I sent you yesterday?

Will

RE: BUG report about ipt_do_table( )

[Wang, Yalin]

Hi  Will, 

I am happy to notify that our stability test has passed,
And this Crash don't happen again,
So seems this patch work now .

We has merged it into our release SW .
Could I know if this patch will be delivered into kernel
Mainline by you ? 


Thanks again !

-----Original Message-----
From: Wang, Yalin 
Sent: Friday, October 11, 2013 7:15 PM
To: 'Will Deacon'
Cc: 'linux-arm-msm-owner@vger.kernel.org'; linux-kernel@vger.kernel.org; Peng, Arthur; Zhang, Bojie; Gu, Youcai 1 (EXT); Alevoor, Raghavendra 2
Subject: RE: BUG report about ipt_do_table( )

Hi Will,

Thanks for your clarification .

I have merged the patch,
And need wait some time to get
The test result ,
You know that this BUG is not easy to reproduce, It's very infrequent.
Maybe we need run several times stability test to Make sure the patch works .

I will update to you the result as soon as possible !

Thanks 

-----Original Message-----
From: Will Deacon [mailto:will.deacon@arm.com]
Sent: Friday, October 11, 2013 7:03 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-owner@vger.kernel.org'; linux-kernel@vger.kernel.org; Peng, Arthur; Zhang, Bojie; Gu, Youcai 1 (EXT); Alevoor, Raghavendra 2
Subject: Re: BUG report about ipt_do_table( )

On Fri, Oct 11, 2013 at 02:50:24AM +0100, Wang, Yalin wrote:
> Hi  Will,

Hello again,

> Maybe I know your meaning ,
> If it use spinlock to protected the shared data, The bug will not 
> happen, because spinlock will Use DSB( )  to sync .

Actually, the dsb is for something else (the sev). It is the smp_mb() call which guarantees the ordering of critical sections with respect to spinlock operations.

> Unluckily, here, it use a special seqcount_t( ) (see get_counters( )
> function)

Well, there is a comment about a write_lock being held, so you should be ok if that's true. The issue I saw was with the newinfo population, as I described in my earlier mail.

> To make sure there is no others using the old data, Before release the 
> old data, this is much like RCU Work, but RCU use rcu_assign_pointer(
> ) --> Which use smp_wmb( ) , so it's safe,  am I right ?

RCU is safe. There are *many* weakly ordered architectures on which Linux runs, so I don't think you have to worry too much about the core data structures and locking/synchronisation/atomic primitives. The major scope for errors is in lockless code, where the barrier usage is explicit.

> In my patch, I use mb( ), because this macro Is DSB( ) , while 
> smp_wmb( ) is DMS( ), I just think DSB is much strict than DMS, mmm..
> so , DSM( )  or DMS ( )  are both ok ?

I think you're getting confused with your barriers. We have two memory barriers on ARM: dmb and dsb. dmb is sufficient to enforce ordering of observability. dsb is used to enforce completion.

> The whitepaper I use is here:
> https://www.google.com/#q=cortex+a15+microarchitecture
> 
> the first: [PDF] Exploring the Design of the Cortex-A15 Processor - 
> ARM
> 
> I just search in Google, and you know that qcom don't release Much 
> document about its krait cpu's micro architecture details, I just use
> cortex-a15 for a reference, I am not sure if their pipeline ( 
> load/store unit) are the same,

I think the lawyers would have a field day if the pipelines were the same!
You really can't use an A15 slide-deck to infer micro-architectural details about Krait.

Please can you test the patch I sent you yesterday?

Will

Re: BUG report about ipt_do_table( )

[Will Deacon]

On Thu, Oct 17, 2013 at 02:51:34AM +0100, Wang, Yalin wrote:
> Hi  Will, 

Hello,

> I am happy to notify that our stability test has passed,
> And this Crash don't happen again,
> So seems this patch work now .

Great!

> We has merged it into our release SW .
> Could I know if this patch will be delivered into kernel
> Mainline by you ? 

Sure, can I add your tested-by please?

Will

RE: BUG report about ipt_do_table( )

[Wang, Yalin]

hi   Will

yes, of course .

by the way, could you send me your delivered commit link after you submit ?

Thanks

-----Original Message-----
From: Will Deacon [mailto:will.deacon@arm.com] 
Sent: Thursday, October 17, 2013 6:41 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-owner@vger.kernel.org'; 'linux-kernel@vger.kernel.org'; Peng, Arthur; Zhang, Bojie; Gu, Youcai 1 (EXT); Alevoor, Raghavendra 2
Subject: Re: BUG report about ipt_do_table( )

On Thu, Oct 17, 2013 at 02:51:34AM +0100, Wang, Yalin wrote:
> Hi  Will,

Hello,

> I am happy to notify that our stability test has passed, And this 
> Crash don't happen again, So seems this patch work now .

Great!

> We has merged it into our release SW .
> Could I know if this patch will be delivered into kernel Mainline by 
> you ?

Sure, can I add your tested-by please?

Will