[3/3] netfilter: x_tables: Use correct memory barriers.
diff mbox series

Message ID 20210304013116.8420-4-mark.tomlinson@alliedtelesis.co.nz
State Accepted
Commit 175e476b8cdf2a4de7432583b49c871345e4f8a1
Headers show
Series
  • Don't use RCU for x_tables synchronization
Related show

Commit Message

Mark Tomlinson March 4, 2021, 1:31 a.m. UTC
When a new table value was assigned, it was followed by a write memory
barrier. This ensured that all writes before this point would complete
before any writes after this point. However, to determine whether the
rules are unused, the sequence counter is read. To ensure that all
writes have been done before these reads, a full memory barrier is
needed, not just a write memory barrier. The same argument applies when
incrementing the counter, before the rules are read.

Changing to using smp_mb() instead of smp_wmb() fixes the kernel panic
reported in cc00bcaa5899, while still maintaining the same speed of
replacing tables.

Fixes: 7f5c6d4f665b ("netfilter: get rid of atomic ops in fast path")
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 include/linux/netfilter/x_tables.h | 2 +-
 net/netfilter/x_tables.c           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Florian Westphal March 4, 2021, 7:46 a.m. UTC | #1
Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz> wrote:
> When a new table value was assigned, it was followed by a write memory
> barrier. This ensured that all writes before this point would complete
> before any writes after this point. However, to determine whether the
> rules are unused, the sequence counter is read. To ensure that all
> writes have been done before these reads, a full memory barrier is
> needed, not just a write memory barrier. The same argument applies when
> incrementing the counter, before the rules are read.
> 
> Changing to using smp_mb() instead of smp_wmb() fixes the kernel panic
> reported in cc00bcaa5899,

Can you reproduce the crashes without this change?

> while still maintaining the same speed of replacing tables.

How much of an impact is the MB change on the packet path?

Please also CC authors of the patches you want reverted when reposting.
Mark Tomlinson March 5, 2021, 3:30 a.m. UTC | #2
On Thu, 2021-03-04 at 08:46 +0100, Florian Westphal wrote:
> Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz> wrote:
> > Changing to using smp_mb() instead of smp_wmb() fixes the kernel panic
> > reported in cc00bcaa5899,
> 
> Can you reproduce the crashes without this change?

Yes. In our test environment we were seeing a kernel panic approx. twice
a day, with a similar output to that shown in Subash's patch (cc00bcaa5899).
With this patch we are not seeing any issue. The CPU is a dual-core ARM
Cortex-A9.

> > How much of an impact is the MB change on the packet path?

I will run our throughput tests and get these results.

I have a script which makes around 200 calls to iptables. This was taking
11.59s and now is back to 1.16s.

Patch
diff mbox series

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index 5deb099d156d..8ec48466410a 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -376,7 +376,7 @@  static inline unsigned int xt_write_recseq_begin(void)
 	 * since addend is most likely 1
 	 */
 	__this_cpu_add(xt_recseq.sequence, addend);
-	smp_wmb();
+	smp_mb();
 
 	return addend;
 }
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index af22dbe85e2c..a2b50596b87e 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1387,7 +1387,7 @@  xt_replace_table(struct xt_table *table,
 	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