Re: [PATCH nf] x_tables: Properly close read section with read_seqcount_retry

[Florian Westphal <fw@strlen.de>]

subashab@codeaurora.org <subashab@codeaurora.org> wrote:
> I have tried the following to ensure the instruction ordering of private
> assignment and I haven't seen the crash so far.
> 
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index af22dbe..2a4f6b3 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1389,6 +1389,9 @@ xt_replace_table(struct xt_table *table,
>         /* make sure all cpus see new ->private value */
>         smp_wmb();
> 
> +       /* make sure the instructions above are actually executed */
> +       smp_mb();
> +

This looks funny, I'd rather have s/smp_wmb/smp_mb instead of yet
another one.

> I assume we would need the following changes to address this.

Yes, something like this.  More comments below.

> diff --git a/include/linux/netfilter/x_tables.h
> b/include/linux/netfilter/x_tables.h
> index 5deb099..7ab0e4f 100644
> --- a/include/linux/netfilter/x_tables.h
> +++ b/include/linux/netfilter/x_tables.h
> @@ -227,7 +227,7 @@ struct xt_table {
>  	unsigned int valid_hooks;
> 
>  	/* Man behind the curtain... */
> -	struct xt_table_info *private;
> +	struct xt_table_info __rcu *private;
> 
>  	/* Set this to THIS_MODULE if you are a module, otherwise NULL */
>  	struct module *me;
> diff --git a/net/ipv4/netfilter/arp_tables.c
> b/net/ipv4/netfilter/arp_tables.c
> index d1e04d2..6a2b551 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -203,7 +203,7 @@ unsigned int arpt_do_table(struct sk_buff *skb,
> 
>  	local_bh_disable();
>  	addend = xt_write_recseq_begin();
> -	private = READ_ONCE(table->private); /* Address dependency. */
> +	private = rcu_access_pointer(table->private);

The three _do_table() functions need to use rcu_dereference().

>  	cpu     = smp_processor_id();
>  	table_base = private->entries;
>  	jumpstack  = (struct arpt_entry **)private->jumpstack[cpu];
> @@ -649,7 +649,7 @@ static struct xt_counters *alloc_counters(const struct
> xt_table *table)
>  {
>  	unsigned int countersize;
>  	struct xt_counters *counters;
> -	const struct xt_table_info *private = table->private;
> +	const struct xt_table_info *private = rcu_access_pointer(table->private);

This looks wrong.  I know its ok, but perhaps its better to add this:

struct xt_table_info *xt_table_get_private_protected(const struct xt_table *table)
{
 return rcu_dereference_protected(table->private, mutex_is_locked(&xt[table->af].mutex));
}
EXPORT_SYMBOL(xt_table_get_private_protected);

to x_tables.c.

If you dislike this extra function, add

#define xt_table_get_private_protected(t) rcu_access_pointer((t)->private)

in include/linux/netfilter/x_tables.h, with a bit fat comment telling
that the xt table mutex must be held.

But I'd rather have/use the helper function as it documents when its
safe to do this (and there will be splats if misused).

> index af22dbe..2e6c09c 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1367,7 +1367,7 @@ xt_replace_table(struct xt_table *table,
> 
>  	/* Do the substitution. */
>  	local_bh_disable();
> -	private = table->private;
> +	private = rcu_access_pointer(table->private);

AFAICS the local_bh_disable/enable calls can be removed too after this,
if we're interrupted by softirq calling any of the _do_table()
functions changes to the xt seqcount do not matter anymore.

>       /*
>        * Even though table entries have now been swapped, other CPU's

We need this additional hunk to switch to rcu for replacement/sync, no?

-       local_bh_enable();
-
-       /* ... so wait for even xt_recseq on all cpus */
-       for_each_possible_cpu(cpu) {
-               seqcount_t *s = &per_cpu(xt_recseq, cpu);
-               u32 seq = raw_read_seqcount(s);
-
-               if (seq & 1) {
-                       do {
-                               cond_resched();
-                               cpu_relax();
-                       } while (seq == raw_read_seqcount(s));
-               }
-       }
+       synchronize_rcu();