Re: [PATCH v4 12/12] sched,signal,ptrace: Rework TASK_TRACED, TASK_STOPPED state

Sven Schnelle
2022-07-06T07:58:55Z

Hi Peter,

Peter Zijlstra <peterz@infradead.org> writes:

> On Tue, Jul 05, 2022 at 07:28:49PM +0200, Sven Schnelle wrote:
>> Sven Schnelle <svens@linux.ibm.com> writes:
>
>> I think there's a race in ptrace_check_attach(). It first calls
>> ptrace_freeze_task(), which checks whether JOBCTL_TRACED is set.
>> If it is (and a few other conditions match) it will set ret = 0.
>>
>> Later outside of siglock and tasklist_lock it will call
>> wait_task_inactive, assuming the target is in TASK_TRACED, but it isn't.
>>
>> ptrace_stop(), which runs on another CPU, does:
>>
>> set_special_state(TASK_TRACED);
>> current->jobctl |= JOBCTL_TRACED;
>>
>> which looks ok on first sight, but in this case JOBCTL is already set,
>> so the reading CPU will immediately move on to wait_task_inactive(),
>> before JOBCTL_TRACED is set. I don't know whether this is a valid
>> combination. I never looked into JOBCTL_* semantics, but i guess now
>> is a good time to do so. I added some debugging statements, and that
>> gives:
>>
>> [ 86.218488] kill_chi-300545 2d.... 79990135us : ptrace_stop: state 8
>> [ 86.218492] kill_chi-300545 2d.... 79990136us : signal_generate: sig\x17 errno=0 code=4 comm=strace pid00542 grp=1 res=1
>> [ 86.218496] kill_chi-300545 2d.... 79990136us : sched_stat_runtime: comm=kill_child pid00545 runtime058 [ns] vruntime`6165713178 [ns]
>> [ 86.218500] kill_chi-300545 2d.... 79990136us : sched_switch:
>> prev_comm=kill_child prev_pid00545 prev_prio\x120 prev_state=t =>
>> next_comm=swapper/2 next_pid=0 next_prio\x120
>> [ 86.218504] strace-300542 7..... 79990139us : sys_ptrace -> 0x50
>> [ 86.218508] strace-300542 7..... 79990139us : sys_write(fd: 2, buf: 2aa198f7ad0, count: 12)
>> [ 86.218512] strace-300542 7..... 79990140us : sys_write -> 0x12
>> [ 86.218515] <idle>-0 6dNh.. 79990140us : sched_wakeup: comm=kill_child pid43805 prio\x120 target_cpu\06
>> [ 86.218519] <idle>-0 6d.... 79990140us : sched_switch:
>> prev_comm=swapper/6 prev_pid=0 prev_prio\x120 prev_state=R =>
>> next_comm=kill_child next_pid43805 next_prio\x120
>> [ 86.218524] strace-300542 7..... 79990140us : sys_write(fd: 2, buf: 2aa198f7ad0, count: 19)
>> [ 86.218527] strace-300542 7..... 79990141us : sys_write -> 0x19
>> [ 86.218531] kill_chi-343805 6..... 79990141us : sys_sched_yield -> 0xffffffffffffffda
>> [ 86.218535] strace-300542 7..... 79990141us : sys_ptrace(request: 18, pid: 53efd, addr: 0, data: 0)
>> [ 86.218539] kill_chi-343805 6d.... 79990141us : signal_deliver: sig=9 errno=0 code=0 sa_handler=0 sa_flags=0
>> [ 86.218543] strace-300542 7d.... 79990141us : ptrace_check_attach: task_is_traced: 1, fatal signal pending: 0
>> [ 86.218547] strace-300542 7..... 79990141us : ptrace_check_attach: child->pid = 343805, child->__flags=0
>> [ 86.218551] kill_chi-343805 6d.... 79990141us : ptrace_stop: JOBCTL_TRACED already set, state=0 <------ valid combination of flags?
>
> Yeah, that's not supposed to be so. JOBCTL_TRACED is supposed to follow
> __TASK_TRACED for now. Set when __TASK_TRACED, cleared when
> TASK_RUNNING.
>
> Specifically {ptrace_,}signal_wake_up() in signal.h clear JOBCTL_TRACED
> when they would wake a __TASK_TRACED task.

try_to_wake_up() clears TASK_TRACED in this case because a signal
(SIGKILL) has to be delivered. As a test I put the following change
on top, and it "fixes" the problem:

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index da0bf6fe9ecd..f2e0f5e70e77 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4141,6 +4149,9 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
* TASK_WAKING such that we can unlock p->pi_lock before doing the
* enqueue, such as ttwu_queue_wakelist().
*/
+ if (p->__state & TASK_TRACED)
+ trace_printk("clearing TASK_TRACED 2\n");
+ p->jobctl &= ~JOBCTL_TRACED;
WRITE_ONCE(p->__state, TASK_WAKING);

/*

There are several places where the state is changed from TASK_TRACED to
something else without clearing JOBCTL_TRACED.