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

[Will Drewry]

On Wed, Jan 15, 2014 at 1:04 PM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> On 01/14, Will Drewry wrote:
>>
>> On Tue, Jan 14, 2014 at 1:21 PM, Oleg Nesterov <oleg@xxxxxxxxxx> 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.

Yeah - not at all.  I'll rethink it. I was too excited about how easy
is_ancestor works, but the locking is really the hard part.

> 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).

Yup - I was thinking of tasklist_lock as a non-multi-reader lock,
which is wrong.
The task_lock(current) would clearly cover that case, but I need to walk through
all the interactions paying more attention to the lock being used.

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

It certainly needs to be better applied :)

thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/