On 07/10, Kees Cook wrote:

On Wed, Jul 9, 2014 at 11:55 AM, Oleg Nesterov [off-list ref] wrote:

quoted

On 07/09, Oleg Nesterov wrote:

quoted

On 06/27, Kees Cook wrote:

quoted

 static u32 seccomp_run_filters(int syscall)
 {
-   struct seccomp_filter *f;
+   struct seccomp_filter *f = ACCESS_ONCE(current->seccomp.filter);

I am not sure...

This is fine if this ->filter is the 1st (and only) one, in this case
we can rely on rmb() in the caller.

But the new filter can be installed at any moment. Say, right after that
rmb() although this doesn't matter. Either we need smp_read_barrier_depends()
after that, or smp_load_acquire() like the previous version did?

Wait... and it seems that seccomp_sync_threads() needs smp_store_release()
when it sets thread->filter = current->filter by the same reason?

OTOH. smp_store_release() in seccomp_attach_filter() can die, "current"
doesn't need a barrier to serialize with itself.

I have lost track of what you're suggesting to change. :)

Perhaps I am just trying to confuse you and myself ;)

But,

Since rmb() happens before run_filters, isn't the ACCESS_ONCE
sufficient?

Yes. But see above. ACCESS_ONCE is sufficient if we read the first filter
installed by another thread, in this case rmb() pairs with mb_before_atomic()
before set_bit(TIF_SECCOMP).

IOW, if this threads sees TIF_SECCOMP, it should also see all modifications
which were done before set_bit, including the data in ->filter points to.

We only care that TIF_SECCOMP, mode, and some filter is
valid. In a tsync thread race, it's okay to use not use the deepest
filter node in the list,

Yes, it is fine if we miss yet another filter which was just installed by
another thread.

But, unless I missed something, the problem is that we can get this new
filter.

Just to simplify. Suppose TIF_SECCOMP was set a long ago. This thread
has a single filter F1 and it enters seccomp_run_filters().

Right before it does ACCESS_ONCE() to read the pointer, another thread
does seccomp_sync_threads() and sets .filter = F2.

If ACCESS_ONCE() returns F1 - everything is fine. But it can see the new
pointer F2, and in this case we need a barrier to ensure that, say,
LOAD(F2->prog) will see all the preceding changes in this memory.

Oleg.