On Tue, Jan 14, 2014 at 1:21 PM, Oleg Nesterov <[email protected]> wrote: > On 01/13, Will Drewry wrote: >> >> +static pid_t seccomp_sync_threads(void) >> +{ >> + struct task_struct *thread, *caller; >> + pid_t failed = 0; >> + thread = caller = current; >> + >> + read_lock(&tasklist_lock); >> + if (thread_group_empty(caller)) >> + goto done; >> + while_each_thread(caller, thread) { >> + task_lock(thread); > > perhaps we take task_lock() to serialize with another caller of > seccomp_sync_threads()...
Sorry for the patch being unclear! The task_lock is meant to protect against the assignment of a seccomp.filter pointer which was meant to protect against the target task calling seccomp_attach_filter while another task is changing its filter pointer. > If yes, then perhaps you can use ->siglock instead of tasklist_lock > and do not use task_lock(). It would be even better to rely on rcu, > but: Exactly. The siglock seems like it makes more sense. >> + 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. It should still get_seccomp_filter() its own reference to the filter. I'm not clear if it's possible to ensure that there is no pathological condition where a thread races and is created without being synchronized. I'll see if the siglock helps here and walk the clone() code again to see what else I missed. thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
