From: "Eric W. Biederman" <ebiederm@xmission.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Oleg Nesterov <oleg@redhat.com>,
Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
linux-kernel@vger.kernel.org, Ben Segall <bsegall@google.com>,
Daniel Bristot de Oliveira <bristot@redhat.com>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
Ingo Molnar <mingo@redhat.com>,
Juri Lelli <juri.lelli@redhat.com>, Mel Gorman <mgorman@suse.de>,
Steven Rostedt <rostedt@goodmis.org>,
Thomas Gleixner <tglx@linutronix.de>,
Vincent Guittot <vincent.guittot@linaro.org>
Subject: Re: [PATCH v2] ptrace: fix ptrace vs tasklist_lock race on PREEMPT_RT.
Date: Fri, 08 Apr 2022 14:40:42 -0500 [thread overview]
Message-ID: <874k332wjp.fsf@email.froward.int.ebiederm.org> (raw)
In-Reply-To: <20220408090908.GO2731@worktop.programming.kicks-ass.net> (Peter Zijlstra's message of "Fri, 8 Apr 2022 11:09:08 +0200")
Peter Zijlstra <peterz@infradead.org> writes:
> On Thu, Apr 07, 2022 at 05:50:39PM -0500, Eric W. Biederman wrote:
>> Given that fundamentally TASK_WAKEKILL must be added in ptrace_stop and
>> removed in ptrace_attach I don't see your proposed usage of jobctl helps
>> anything fundamental.
>>
>> I suspect somewhere there is a deep trade-off between complicating
>> the scheduler to have a very special case for what is now
>> TASK_RTLOCK_WAIT, and complicating the rest of the code with having
>> TASK_RTLOCK_WAIT in __state and the values that should be in state
>> stored somewhere else.
>
> The thing is; ptrace is a special case. I feel very strongly we should
> not complicate the scheduler/wakeup path for something that 'never'
> happens.
I was going to comment that I could not understand how the saved_state
mechanism under PREEMPT_RT works. Then I realized that wake_up_process
and wake_up_state call try_to_wake_up which calls ttwu_state_match which
modifies saved_state.
The options appear to be that either ptrace_freeze_traced modifies
__state/state to remove TASK_KILLABLE. Or that something clever happens
in ptrace_freeze_traced that guarantees the task does not wake
up. Something living in kernel/sched/* like wait_task_inactive.
I can imagine adding add a loop around freezable_schedule in
ptrace_stop. That does something like:
do {
freezable_schedule();
} while (current->jobctl & JOBCTL_PTRACE_FREEZE);
Unfortunately after a SIGKILL is delivered the process will never sleep
unless there is a higher priority process to preempt it. So I don't
think that is a viable solution.
What ptrace_freeze_traced and ptrace_unfreeze_traced fundamentally need
is that the process to not do anything interesting, so that the tracer
process can modify the process and it's task_struct.
That need is the entire reason ptrace does questionable things with
with __state.
So if we can do something better perhaps with a rewritten freezer it
would be a general code improvement.
The ptrace code really does want TASK_KILLABLE semantics the entire time
a task is being manipulated by the ptrace system call. The code in
ptrace_unfreeze_traced goes through some gymnastics to detect if a
process was killed while traced (AKA to detect a missed SIGKILL)
and to use wake_up_state to make the task runnable instead of putting
it back in TASK_TRACED.
So really all that is required is a way to ask the scheduler to just
not schedule the process until the ptrace syscall completes and calls
ptrace_unfreeze_traced.
Eric
next prev parent reply other threads:[~2022-04-08 19:41 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-02 21:04 [PATCH] ptrace: fix ptrace vs tasklist_lock race on PREEMPT_RT Sebastian Andrzej Siewior
2022-03-14 9:27 ` Sebastian Andrzej Siewior
2022-03-14 18:54 ` Oleg Nesterov
2022-03-15 8:31 ` Sebastian Andrzej Siewior
2022-03-15 14:29 ` Oleg Nesterov
2022-03-16 8:23 ` Sebastian Andrzej Siewior
2022-03-31 14:25 ` [PATCH v2] " Sebastian Andrzej Siewior
2022-04-04 16:13 ` Oleg Nesterov
2022-04-05 8:34 ` Oleg Nesterov
2022-04-05 10:10 ` Peter Zijlstra
2022-04-05 10:29 ` Oleg Nesterov
2022-04-05 11:28 ` Peter Zijlstra
2022-04-07 12:13 ` Peter Zijlstra
2022-04-07 17:51 ` Eric W. Biederman
2022-04-07 22:50 ` Eric W. Biederman
2022-04-08 9:09 ` Peter Zijlstra
2022-04-08 19:40 ` Eric W. Biederman [this message]
2022-04-08 20:06 ` Peter Zijlstra
2022-04-11 11:35 ` Peter Zijlstra
2022-04-11 13:44 ` Eric W. Biederman
2022-04-11 17:07 ` Peter Zijlstra
2022-04-12 11:59 ` Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874k332wjp.fsf@email.froward.int.ebiederm.org \
--to=ebiederm@xmission.com \
--cc=bigeasy@linutronix.de \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=dietmar.eggemann@arm.com \
--cc=juri.lelli@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=oleg@redhat.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).