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

[subashab@codeaurora.org]

> I'm fine with this change but AFAIU this is just a cleanup since
> this part isn't a read-sequence as no  'shared state' is accessed/read 
> between
> the seqcount begin and the do{}while. smb_rmb placement should not 
> matter here.
> 
> Did I miss anything?
> 
> Thanks.

Hi Florian

To provide more background on this, we are seeing occasional crashes in 
a
regression rack in the packet receive path where there appear to be
some rules being modified concurrently.

Unable to handle kernel NULL pointer dereference at virtual
address 000000000000008e
pc : ip6t_do_table+0x5d0/0x89c
lr : ip6t_do_table+0x5b8/0x89c
ip6t_do_table+0x5d0/0x89c
ip6table_filter_hook+0x24/0x30
nf_hook_slow+0x84/0x120
ip6_input+0x74/0xe0
ip6_rcv_finish+0x7c/0x128
ipv6_rcv+0xac/0xe4
__netif_receive_skb+0x84/0x17c
process_backlog+0x15c/0x1b8
napi_poll+0x88/0x284
net_rx_action+0xbc/0x23c
__do_softirq+0x20c/0x48c

We found that ip6t_do_table was reading stale values from the READ_ONCE
leading to use after free when dereferencing per CPU jumpstack values.

[https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/net/ipv6/netfilter/ip6_tables.c?h=v5.4.77#n282]
	addend = xt_write_recseq_begin();
	private = READ_ONCE(table->private); /* Address dependency. */

We were able to confirm that the xt_replace_table & __do_replace
had already executed by logging the seqcount values. The value of
seqcount at ip6t_do_table was 1 more than the value at xt_replace_table.
The seqcount read at xt_replace_table also showed that it was an even 
value
and hence meant that there was no conccurent writer instance 
(xt_write_recseq_begin)
at that point.

[https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/net/netfilter/x_tables.c?h=v5.4.77#n1401]
		u32 seq = raw_read_seqcount(s);

This means that table assignment at xt_replace_table did not take effect
as expected.

[https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/net/netfilter/x_tables.c?h=v5.4.77#n1386]
	smp_wmb();
	table->private = newinfo;

	/* make sure all cpus see new ->private value */
	smp_wmb();

We want to know if this barrier usage is as expected here.
Alternatively, would changing this help here -

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 525f674..417ea1b 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1431,11 +1431,11 @@ xt_replace_table(struct xt_table *table,
          * Ensure contents of newinfo are visible before assigning to
          * private.
          */
-       smp_wmb();
-       table->private = newinfo;
+       smp_mb();
+       WRITE_ONCE(table->private, newinfo);

         /* make sure all cpus see new ->private value */
-       smp_wmb();
+       smp_mb();

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