Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 8 Jul 2021 14:02:14 +0200
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
Message-ID: <20210708120213.GA29937@redhat.com>
References: <20210705151019.989929-1-vladimir.divjak@bmw.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210705151019.989929-1-vladimir.divjak@bmw.de>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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.