Re: [PATCH v3] fix race copy_process() vs de_thread()

From: Hiroshi Shimamoto
Date: Mon Aug 24 2009 - 20:21:03 EST


Oleg Nesterov wrote:
> Spotted by Hiroshi Shimamoto who also provided the test-case below.
>
> copy_process() pathes use signal->count as a reference counter, but
> it is not. This test case
>
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <unistd.h>
> #include <stdio.h>
> #include <errno.h>
> #include <pthread.h>
>
> void *null_thread(void *p)
> {
> for (;;)
> sleep(1);
>
> return NULL;
> }
>
> void *exec_thread(void *p)
> {
> execl("/bin/true", "/bin/true", NULL);
>
> return null_thread(p);
> }
>
> int main(int argc, char **argv)
> {
> for (;;) {
> pid_t pid;
> int ret, status;
>
> pid = fork();
> if (pid < 0)
> break;
>
> if (!pid) {
> pthread_t tid;
>
> pthread_create(&tid, NULL, exec_thread, NULL);
> for (;;)
> pthread_create(&tid, NULL, null_thread, NULL);
> }
>
> do {
> ret = waitpid(pid, &status, 0);
> } while (ret == -1 && errno == EINTR);
> }
>
> return 0;
> }
>
> quickly creates the unkillable task.
>
> If copy_process(CLONE_THREAD) races with de_thread()
> copy_signal()->atomic(signal->count) breaks the signal->notify_count
> logic, and the execing thread can hang forever in kernel space.
>
> Change copy_process() to increment count/live only when we know for
> sure we can't fail. In this case the forked thread will take care
> of its reference to signal correctly.
>
> If copy_process() fails, check CLONE_THREAD flag. If it it set - do
> nothing, the counters were not changed and current belongs to the same
> thread group. If it is not set, ->signal must be released in any case
> (and ->count must be == 1), the forked child is the only thread in the
> thread group.
>
> We need more cleanups here, in particular signal->count should not be
> used by de_thread/__exit_signal at all. This patch only fixes the bug.
>
> Reported-by: Hiroshi Shimamoto <h-shimamoto@xxxxxxxxxxxxx>
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>

Nice fix!
Tested-by: Hiroshi Shimamoto <h-shimamoto@xxxxxxxxxxxxx>

Thanks,
Hiroshi
--
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/