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

[Florian Westphal <fw@strlen.de>]

Will Deacon <will@kernel.org> wrote:
> On Mon, Nov 16, 2020 at 07:20:28PM +0100, Florian Westphal wrote:
> > subashab@codeaurora.org <subashab@codeaurora.org> wrote:
> > > > > Unfortunately we are seeing it on ARM64 regression systems which
> > > > > runs a
> > > > > variety of
> > > > > usecases so the exact steps are not known.
> > > > 
> > > > Ok.  Would you be willing to run some of those with your suggested
> > > > change to see if that resolves the crashes or is that so rare that this
> > > > isn't practical?
> > > 
> > > I can try that out. Let me know if you have any other suggestions as well
> > > and I can try that too.
> > > 
> > > I assume we cant add locks here as it would be in the packet processing
> > > path.
> > 
> > Yes.  We can add a synchronize_net() in xt_replace_table if needed
> > though, before starting to put the references on the old ruleset
> > This would avoid the free of the jumpstack while skbs are still
> > in-flight.
> 
> I tried to make sense of what's going on here, and it looks to me like
> the interaction between xt_replace_table() and ip6t_do_table() relies on
> store->load order that is _not_ enforced in the code.
> 
> xt_replace_table() does:
> 
> 	table->private = newinfo;
> 
> 	/* make sure all cpus see new ->private value */
> 	smp_wmb();
> 
> 	/*
> 	 * Even though table entries have now been swapped, other CPU's
> 	 * may still be using the old entries...
> 	 */
> 	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));
> 		}
> 	}
> 
> and I think the idea here is that we swizzle the table->private pointer
> to point to the new data, and then wait for all pre-existing readers to
> finish with the old table (isn't this what RCU is for?).

Yes and yes.   Before the current 'for_each_possible_cpu() + seqcount
abuse the abuse was just implicit; xt_replace_table did NOT wait for
readers to go away and relied on arp, eb and ip(6)tables to store the
counters back to userspace after xt_replace_table().

This meant a read_seqcount_begin/retry for each counter & eventually
gave complaits from k8s users that x_tables rule replacement was too
slow.

[..]

> Given that this appears to be going wrong in practice, I've included a quick
> hack below which should fix the ordering. However, I think it would be
> better if we could avoid abusing the seqcount code for this sort of thing.

I'd be ok with going via the simpler solution & wait if k8s users
complain that its too slow.  Those memory blobs can be huge so I would
not use call_rcu here.

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index af22dbe85e2c..b5911985d1eb 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1384,7 +1384,7 @@ xt_replace_table(struct xt_table *table,
         * private.
         */
        smp_wmb();
-       table->private = newinfo;
+       WRITE_ONCE(table->private, newinfo);
 
        /* make sure all cpus see new ->private value */
        smp_wmb();
@@ -1394,19 +1394,7 @@ xt_replace_table(struct xt_table *table,
         * may still be using the old entries...
         */
        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();
 
        audit_log_nfcfg(table->name, table->af, private->number,
                        !private->number ? AUDIT_XT_OP_REGISTER :