RE: BUG report about ipt_do_table( )

2013-10-17 Thread 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.dea...@arm.com] 
Sent: Thursday, October 17, 2013 6:41 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-ow...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: BUG report about ipt_do_table( )

2013-10-17 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: BUG report about ipt_do_table( )

2013-10-16 Thread 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-ow...@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.dea...@arm.com]
Sent: Friday, October 11, 2013 7:03 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-ow...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: BUG report about ipt_do_table( )

2013-10-11 Thread 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.dea...@arm.com] 
Sent: Friday, October 11, 2013 7:03 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-ow...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: BUG report about ipt_do_table( )

2013-10-11 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: BUG report about ipt_do_table( )

2013-10-10 Thread Wang, Yalin
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.dea...@arm.com] 
Sent: Thursday, October 10, 2013 10:19 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-ow...@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


Re: BUG report about ipt_do_table( )

2013-10-10 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: BUG report about ipt_do_table( )

2013-10-10 Thread Wang, Yalin
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.dea...@arm.com] 
Sent: Thursday, October 10, 2013 7:03 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-ow...@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 8b03

Re: BUG report about ipt_do_table( )

2013-10-10 Thread 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: BUG report about ipt_do_table( )

2013-10-10 Thread 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=M8960ANLYA26144005
https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/net/netfilter/x_tables.c?id=M8960ANLYA26144005



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.dea...@arm.com] 
Sent: Thursday, October 10, 2013 5:48 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-ow...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: BUG report about ipt_do_table( )

2013-10-10 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/