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:   Fri, 9 Jul 2021 21:15:29 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
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>, <oleg@redhat.com>
Subject: Re: [PATCH] coredump: allow PTRACE_ATTACH to coredump user mode
 helper
Message-Id: <20210709211529.59d4263a4780da8de14f351b@linux-foundation.org>
In-Reply-To: <20210705151019.989929-1-vladimir.divjak@bmw.de>
References: <20210705151019.989929-1-vladimir.divjak@bmw.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 5 Jul 2021 17:10:19 +0200 Vladimir Divjak <vladimir.divjak@bmw.de> wrote:

> This patch allows the coredump user mode helper process,
> if one is configured (in core_pattern),
> to perform ptrace operations on the dying process
> whose cordump it's handling.
> 
> The user mode helper process is expected to do so
> before consuming the coredump data from the pipe,
> and thereby, before the dying process is reaped by kernel.
> 
> Issuing a PTRACE_ATTACH request will pause the coredumping
> for that task until ptrace operation is finished.
> 
> The user mode helper process is also expected to
> issue a PTRACE_CONT request to the dying process,
> when it is done ptracing it, signaling the dying process
> coredumping can be resumed.
> 
> * 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.
> 
> * Solution:
> Allow the user mode coredump helper process
> to perform ptrace on the dying process in order to obtain
> useful information such as user mode stacktrace, and
> thereby greatly improve the offline debugging possibilities
> for such environments.
> 
> * Impact / Risk:
> The user mode helper process is already entrusted
> with handling the coredump data, so allowing it to read or even change
> the dying process memory should not pose an additional risk.
> 
> Furthermore, this change makes coredump emission somewhat slower
> due to the additional step of iterating over the core dump helper list
> and checking if ptrace completion needs to be awaited,
> during coredump emission.
> 

Seems useful.

>
> ...
>
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -41,6 +41,7 @@
>  #include <linux/fs.h>
>  #include <linux/path.h>
>  #include <linux/timekeeping.h>
> +#include <linux/jiffies.h>
>  
>  #include <linux/uaccess.h>
>  #include <asm/mmu_context.h>
> @@ -62,6 +63,64 @@ struct core_name {
>  	int used, size;
>  };
>  
> +DEFINE_MUTEX(cdh_mutex);
> +LIST_HEAD(cdh_list);

I think this could be static?

> +	struct task_struct *task_being_dumped;
> +	struct completion ptrace_done;
> +	pid_t helper_pid;
> +};
>
> ...
>
> @@ -692,9 +751,14 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  		sub_info = call_usermodehelper_setup(helper_argv[0],
>  						helper_argv, NULL, GFP_KERNEL,
>  						umh_pipe_setup, NULL, &cprm);
> -		if (sub_info)
> +		if (sub_info) {
> +			mutex_lock(&cdh_mutex);
>  			retval = call_usermodehelper_exec(sub_info,
>  							  UMH_WAIT_EXEC);
> +			if (!retval)
> +				cdh_link_current_locked(sub_info->pid);
> +			mutex_unlock(&cdh_mutex);

Dumb question: can the usermode helper coredump and then try to take
cdh_mutex, causing a deadlock?

> +		}
>  
>  		kfree(helper_argv);
>  		if (retval) {
>
> ...
>
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c

It seems regrettable that the ptrace code has to be aware of the
coredump code in this fashion.