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
