Re: [PATCH 2/2] sys, seccomp: add PR_SECCOMP_EXT and SECCOMP_EXT_ACT_TSYNC

[Oleg Nesterov <oleg@redhat.com>]

On 01/14, Will Drewry wrote:
>
> On Tue, Jan 14, 2014 at 1:21 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> >> +                     get_seccomp_filter(caller);
> >> +                     /*
> >> +                      * Drop the task reference to the shared ancestor since
> >> +                      * current's path will hold a reference.  (This also
> >> +                      * allows a put before the assignment.)
> >> +                      */
> >> +                     put_seccomp_filter(thread);
> >> +                     thread->seccomp.filter = caller->seccomp.filter;
> >
> > As I said, I do not understand this patch yet, but this looks suspicious.
> >
> > Why we can't race with this thread doing clone(CLONE_THREAD) ? We do
> > not the the new thread yet, but its ->seccomp can be already copied
> > by copy_process(), no?
>
> Yeah I missed that. That said, I think the worst of it would be that
> the new thread
> gets the old filter.

Yes, but this means you can trust SECCOMP_EXT_ACT_TSYNC.

> I'll see if
> the siglock helps
> here and walk the clone() code again to see what else I missed.

No, siglock itself can't help to avoid this race. Unless you move
copy_process()->get_seccomp_filter() under the same lock, and in
this case it should also re-copy ->seccomp. Not nice.

But note task_lock() (or any other per-thread locking) is wrong.
Just look at the code above. We hold task_lock(thread) but not
task_lock(caller). What if another thread calls seccomp_sync_threads()
and changes caller->seccomp right after get_seccomp_filter(caller).

And even get_seccomp_filter() itself becomes racy. I think the
locking is seriously broken in this series.

Oleg.