RE: BUG report about ipt_do_table( )

[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.deacon@xxxxxxx] 
Sent: Thursday, October 10, 2013 7:03 PM
To: Wang, Yalin
Cc: 'linux-arm-msm-owner@xxxxxxxxxxxxxxx'; linux-kernel@xxxxxxxxxxxxxxx; 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‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i