From: Oleg Nesterov <oleg@redhat.com>
To: Vladimir Divjak <vladimir.divjak@bmw.de>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
viro@zeniv.linux.org.uk, mcgrof@kernel.org,
akpm@linux-foundation.org
Subject: Re: [PATCH] coredump: allow PTRACE_ATTACH to coredump user mode helper
Date: Thu, 8 Jul 2021 14:02:14 +0200 [thread overview]
Message-ID: <20210708120213.GA29937@redhat.com> (raw)
In-Reply-To: <20210705151019.989929-1-vladimir.divjak@bmw.de>
On 07/05, Vladimir Divjak wrote:
>
> * Problem description / Rationale:
> In automotive and/or embedded environments,
> the storage capacity to store, and/or
> network capabilities to upload
> a complete core file can easily be a limiting factor,
> making offline issue analysis difficult.
To be honest, I don't like the idea... plus the implementation looks
horrible to me, sorry.
Can't the coredump helper process simply do
ptrace(PTRACE_SEIZE, PTRACE_O_TRACEEXIT), close the pipe, and wait
for PTRACE_EVENT_EXIT ? Then it can use ptrace() as usual.
> +void cdh_unlink_current(void)
> +{
> + struct cdh_entry *entry, *next;
> +
> + mutex_lock(&cdh_mutex);
> + list_for_each_entry_safe(entry, next, &cdh_list, cdh_list_link) {
Why _safe ?
> +bool cdh_ptrace_allowed(struct task_struct *task)
> +{
> + struct cdh_entry *entry;
> +
> + mutex_lock(&cdh_mutex);
> + list_for_each_entry(entry, &cdh_list, cdh_list_link) {
> + if (task_tgid_nr(entry->task_being_dumped) == task_tgid_nr(task)
> + && entry->helper_pid == task_tgid_nr(current)) {
> + reinit_completion(&(entry->ptrace_done));
> + wait_task_inactive(entry->task_being_dumped, 0);
So. IIUC, this assumes that when cdh_ptrace_allowed() returns the dumping
process must be blocked in dump_emit()->wait_for_completion(ptrace_done).
And thus ptrace_attach() can safely do task->state = TASK_TRACED.
But it is possible that __dump_emit() has already failed and task_being_dumped
sleeps in cdh_unlink_current() waiting for cdh_mutex. So it will be running
right after cdh_ptrace_allowed() drops cdh_mutex.
> +struct cdh_entry *cdh_get_entry_for_current(void)
> +{
> + struct cdh_entry *entry;
> +
> + list_for_each_entry(entry, &cdh_list, cdh_list_link) {
> + if (entry->task_being_dumped == current)
> + return entry;
Why is it safe without cdh_mutex ?
> @@ -361,6 +362,8 @@ static int ptrace_attach(struct task_struct *task, long request,
> {
> bool seize = (request == PTRACE_SEIZE);
> int retval;
> + bool core_state = false;
> + bool core_trace_allowed = false;
>
> retval = -EIO;
> if (seize) {
> @@ -392,10 +395,17 @@ static int ptrace_attach(struct task_struct *task, long request,
>
> task_lock(task);
> retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> + if (unlikely(task->mm->core_state))
> + core_state = true;
task->mm can be NULL
> + if (!seize && unlikely(core_state)) {
> + if (cdh_ptrace_allowed(task))
> + core_trace_allowed = true;
> + }
Why !seize ???
What if ptrace_attach() fails after that? Who will wake this task up ?
> + /*
> + * Core state process does not process signals normally.
> + * set directly to TASK_TRACED if allowed by cdh_ptrace_allowed.
> + */
> + if (core_trace_allowed)
> + task->state = TASK_TRACED;
See above.
But even if I missed something, this is wrong no matter what, you should
never change another task's state.
> @@ -821,6 +838,8 @@ static int ptrace_resume(struct task_struct *child, long request,
> {
> bool need_siglock;
>
> + cdh_signal_continue(child);
takes cdh_mutex :/
Oleg.
next prev parent reply other threads:[~2021-07-08 12:02 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-05 15:10 [PATCH] coredump: allow PTRACE_ATTACH to coredump user mode helper Vladimir Divjak
2021-07-08 12:02 ` Oleg Nesterov [this message]
2021-07-10 4:15 ` Andrew Morton
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=20210708120213.GA29937@redhat.com \
--to=oleg@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=vladimir.divjak@bmw.de \
/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).