subashab@xxxxxxxxxxxxxx <subashab@xxxxxxxxxxxxxx> 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();
