Messages in this thread | | | From | "Eric W. Biederman" <> | Date | Mon, 25 Apr 2022 19:38:21 -0500 | Subject | Re: [PATCH v2 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT |
| |
Peter Zijlstra <peterz@infradead.org> writes:
> On Mon, Apr 25, 2022 at 04:35:37PM +0200, Oleg Nesterov wrote: >> On 04/21, Peter Zijlstra wrote: >> > >> > +static void clear_traced_quiesce(void) >> > +{ >> > + spin_lock_irq(¤t->sighand->siglock); >> > + WARN_ON_ONCE(!(current->jobctl & JOBCTL_TRACED_QUIESCE)); >> >> This WARN_ON_ONCE() doesn't look right, the task can be killed right >> after ptrace_stop() sets JOBCTL_TRACED | JOBCTL_TRACED_QUIESCE and >> drops siglock. > > OK, will look at that. > >> > @@ -2290,14 +2303,26 @@ static int ptrace_stop(int exit_code, in >> > /* >> > * Don't want to allow preemption here, because >> > * sys_ptrace() needs this task to be inactive. >> > - * >> > - * XXX: implement read_unlock_no_resched(). >> > */ >> > preempt_disable(); >> > read_unlock(&tasklist_lock); >> > - cgroup_enter_frozen(); >> > + cgroup_enter_frozen(); // XXX broken on PREEMPT_RT !!! >> > + >> > + /* >> > + * JOBCTL_TRACE_QUIESCE bridges the gap between >> > + * set_current_state(TASK_TRACED) above and schedule() below. >> > + * There must not be any blocking (specifically anything that >> > + * touched ->saved_state on PREEMPT_RT) between here and >> > + * schedule(). >> > + * >> > + * ptrace_check_attach() relies on this with its >> > + * wait_task_inactive() usage. >> > + */ >> > + clear_traced_quiesce(); >> >> Well, I think it should be called earlier under tasklist_lock, >> before preempt_disable() above. >> >> We need tasklist_lock to protect ->parent, debugger can be killed >> and go away right after read_unlock(&tasklist_lock). >> >> Still trying to convince myself everything is right with >> JOBCTL_STOPPED/TRACED ... > > Can't do it earlier, since cgroup_enter_frozen() can do spinlock (eg. > use ->saved_state).
There are some other issues in this part of ptrace_stop().
I don't see JOBCTL_TRACED_QUIESCE being cleared "if (!current->ptrace)".
Currently in ptrace_check_attach a parameter of __TASK_TRACED is passed so that wait_task_inactive cane fail if the "!current->ptrace" branch of ptrace_stop is take and ptrace_stop does not stop. With the TASK_FROZEN state it appears that "!current->ptrace" branch can continue and freeze somewhere else and wait_task_inactive could decided it was fine.
I have to run, but hopefully tommorrow I will post the patches that remove the "!current->ptrace" case altogether and basically remove the need for quiesce and wait_task_inactive detecting which branch is taken.
The spinlock in cgroup_enter_frozen remains an issue for PREEMPT_RT. But the rest of the issues are cleared up by using siglock instead of tasklist_lock. Plus the code is just easier to read and understand.
Eric
| |