2007-11-22 16:15:31

by Oleg Nesterov

Subject: [PATCH -mm 1/2] ptrace_stop: fix the race with ptrace detach+attach

If the tracer went away (may_ptrace_stop() failed), ptrace_stop() drops tasklist
and then changes the ->state from TASK_TRACED to TASK_RUNNING.

This can fool another tracer which attaches to us in between. Change the ->state
under tasklist_lock to ensure that ptrace_check_attach() can't wrongly succeed.
Also, remove the unnecessary mb().

Signed-off-by: Oleg Nesterov <[email protected]>

--- PT/kernel/signal.c~1_ptrace_stop 2007-11-21 21:41:02.000000000 +0300
+++ PT/kernel/signal.c 2007-11-22 16:59:35.000000000 +0300
@@ -1628,11 +1628,11 @@ static void ptrace_stop(int exit_code, i
} else {
/*
* By the time we got the lock, our tracer went away.
- * Don't stop here.
+ * Don't drop the lock yet, another tracer may come.
*/
- read_unlock(&tasklist_lock);
- set_current_state(TASK_RUNNING);
+ __set_current_state(TASK_RUNNING);
current->exit_code = nostop_code;
+ read_unlock(&tasklist_lock);
}

/*

2007-11-23 10:24:53

by Alexey Dobriyan

Subject: Re: [PATCH -mm 1/2] ptrace_stop: fix the race with ptrace detach+attach

On Thu, Nov 22, 2007 at 07:14:59PM +0300, Oleg Nesterov wrote:
> If the tracer went away (may_ptrace_stop() failed), ptrace_stop() drops tasklist
> and then changes the ->state from TASK_TRACED to TASK_RUNNING.
>
> This can fool another tracer which attaches to us in between. Change the ->state
> under tasklist_lock to ensure that ptrace_check_attach() can't wrongly succeed.

ptrace_check_attach? Both do read_lock -- can run in parallel, so how can it help?

> --- PT/kernel/signal.c~1_ptrace_stop 2007-11-21 21:41:02.000000000 +0300
> +++ PT/kernel/signal.c 2007-11-22 16:59:35.000000000 +0300
> @@ -1628,11 +1628,11 @@ static void ptrace_stop(int exit_code, i
> } else {
> /*
> * By the time we got the lock, our tracer went away.
> - * Don't stop here.
> + * Don't drop the lock yet, another tracer may come.
> */
> - read_unlock(&tasklist_lock);
> - set_current_state(TASK_RUNNING);
> + __set_current_state(TASK_RUNNING);
> current->exit_code = nostop_code;
> + read_unlock(&tasklist_lock);
> }
>
> /*

2007-11-23 10:56:29

by Oleg Nesterov

Subject: Re: [PATCH -mm 1/2] ptrace_stop: fix the race with ptrace detach+attach

On 11/23, Alexey Dobriyan wrote:
>
> On Thu, Nov 22, 2007 at 07:14:59PM +0300, Oleg Nesterov wrote:
> > If the tracer went away (may_ptrace_stop() failed), ptrace_stop() drops tasklist
> > and then changes the ->state from TASK_TRACED to TASK_RUNNING.
> >
> > This can fool another tracer which attaches to us in between. Change the ->state
> > under tasklist_lock to ensure that ptrace_check_attach() can't wrongly succeed.
>
> ptrace_check_attach? Both do read_lock -- can run in parallel,

Yep.

> so how can it help?

read_lock prevents ptrace_attach(), so the new tracer can't attach
and then do ptrace_check_attach().

Oleg.

2007-12-11 01:36:52

by Roland McGrath

Subject: Re: [PATCH -mm 1/2] ptrace_stop: fix the race with ptrace detach+attach

Your change looks correct to me.


Thanks,
Roland