Re: [PATCH 3/3] netfilter: x_tables: fix ordering of jumpstack allocation and table update

Hi David,

On Wed, Oct 23, 2013 at 10:45:04AM +0100, David Laight wrote:

quoted

Subject: [PATCH 3/3] netfilter: x_tables: fix ordering of jumpstack allocation and table update

...

quoted

Meanwhile, CPU0 is handling the network receive path and ends up in
ipt_do_table, resulting in:

	private = table->private;

	[...]

	jumpstack  = (struct ipt_entry **)private->jumpstack[cpu];

On weakly ordered memory architectures, the writes to table->private
and newinfo->jumpstack from CPU1 can be observed out of order by CPU0.
Furthermore, on architectures which don't respect ordering of address
dependencies (i.e. Alpha), the reads from CPU0 can also be re-ordered.

Which reads might be out of order?
AFAICT they are strongly sequenced because they second depends on the
value read by the first.
So I don't see why the read barrier is needed.

That is why this is a dependent read barrier. Some architectures (e.g.
Alpha) *do* allow dependent reads to be observed out of order, so you can
effectively load the data pointed to by a pointer before you load the
pointer itself!

Take a look at Paul's paper about memory ordering if you're curious:

  http://www.rdrop.com/users/paulmck/scalability/paper/whymb.2009.04.05a.pdf

quoted

-	table->private = newinfo;
 	newinfo->initial_entries = private->initial_entries;
+	/*
+	 * Ensure contents of newinfo are visible before assigning to
+	 * private.
+	 */
+	smp_wmb();
+	table->private = newinfo;

Those writes were in the wrong order on all systems.
Also gcc needs to be told not to reorder the writes even on non-smp
systems (if the code might be pre-empted).
So an asm volatile (:::"memory") is needed there even if no specific
synchronisation instruction is needed.

The smp_* barriers expand to barrier() when !CONFIG_SMP, which gives you the
memory clobber you want.

What I'm *not* 100% sure about is the table freeing path. There is a mutex
there for removing the table from a list, but I'm not sure how we ensure
that there are no parallel readers at that point.

Will