lkml.org 
[lkml]   [2014]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] sys, seccomp: add PR_SECCOMP_EXT and SECCOMP_EXT_ACT_TSYNC
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.



\
 
 \ /
  Last update: 2014-01-15 21:01    [W:0.070 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site