[PATCH] proc: report eip and esp for all threads when coredumping

[PATCH] proc: report eip and esp for all threads when coredumping

[Jan Luebbe]

Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
/proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
reintroduced the feature to fix a regression with userspace core dump
handlers (such as minicoredumper).

Because PF_DUMPCORE is only set for the primary thread, this didn't fix
the original problem for secondary threads. This commit checks
mm->core_state instead, as already done for /proc/<pid>/status in
task_core_dumping(). As we have a mm_struct available here anyway, this
seems to be a clean solution.

Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
---
 fs/proc/array.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 2edbb657f859..b76b1e29fc36 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -462,7 +462,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		 * a program is not able to use ptrace(2) in that case. It is
 		 * safe because the task has stopped executing permanently.
 		 */
-		if (permitted && (task->flags & PF_DUMPCORE)) {
+		if (permitted && (!!mm->core_state)) {
 			if (try_get_task_stack(task)) {
 				eip = KSTK_EIP(task);
 				esp = KSTK_ESP(task);
-- 
2.11.0

Re: [PATCH] proc: report eip and esp for all threads when coredumping

[Alexey Dobriyan]

On Wed, May 22, 2019 at 06:16:14PM +0200, Jan Luebbe wrote:
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -462,7 +462,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>  		 * a program is not able to use ptrace(2) in that case. It is
>  		 * safe because the task has stopped executing permanently.
>  		 */
> -		if (permitted && (task->flags & PF_DUMPCORE)) {
> +		if (permitted && (!!mm->core_state)) {

You don't need both "!!" and "()", it is a regular pointer after all.

Re: [PATCH] proc: report eip and esp for all threads when coredumping

[Andrew Morton]

On Wed, 22 May 2019 18:16:14 +0200 Jan Luebbe <jlu@pengutronix.de> wrote:

> Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
> /proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
> ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
> reintroduced the feature to fix a regression with userspace core dump
> handlers (such as minicoredumper).
> 
> Because PF_DUMPCORE is only set for the primary thread, this didn't fix
> the original problem for secondary threads. This commit checks
> mm->core_state instead, as already done for /proc/<pid>/status in
> task_core_dumping(). As we have a mm_struct available here anyway, this
> seems to be a clean solution.
> 

Could we please have an explicit and complete description of the
end-user visible effect of this change?

It sounds like we should be backporting this into -stable but without
the above info it's hard to determine this.

Re: [PATCH] proc: report eip and esp for all threads when coredumping

[Jan Lübbe]

On Wed, 2019-05-22 at 11:00 -0700, Andrew Morton wrote:
> On Wed, 22 May 2019 18:16:14 +0200 Jan Luebbe <jlu@pengutronix.de> wrote:
> 
> > Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
> > /proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
> > ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
> > reintroduced the feature to fix a regression with userspace core dump
> > handlers (such as minicoredumper).
> > 
> > Because PF_DUMPCORE is only set for the primary thread, this didn't fix
> > the original problem for secondary threads. This commit checks
> > mm->core_state instead, as already done for /proc/<pid>/status in
> > task_core_dumping(). As we have a mm_struct available here anyway, this
> > seems to be a clean solution.
> 
> Could we please have an explicit and complete description of the
> end-user visible effect of this change?

In current mainline, all threads except the main have the
/proc/[pid]/stat fields 'kstkesp' (29, current stack pointer) and
'kstkeip' (30, current instruction pointer) show as 0 even during
coredumping when read by the core dump handler.

minicoredumper for example tries to use this value to find each
thread's stack and tries to dump it, which fails as there is nothing
mapped at 0. The result is that the thread's stack data is missing from
the generated core dump.

With this patch, kstkesp and kstkeip are visible again to the core dump
handler, so the minified core dump contains all stacks again. For a
process running normally, the values are still reported as 0 (as
intended).

> It sounds like we should be backporting this into -stable but without
> the above info it's hard to determine this.

We've been using this patch on 4.19.x for some time, so I agree that
this should be back-ported (fd7d56270b52 is in 4.14).


Andrew, should I send a v2 with Alexey's fix squashed and an updated
commit message?

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] proc: report eip and esp for all threads when coredumping

[John Ogness]

On 2019-05-22, Jan Luebbe <jlu@pengutronix.de> wrote:
> Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
> /proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
> ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
> reintroduced the feature to fix a regression with userspace core dump
> handlers (such as minicoredumper).
>
> Because PF_DUMPCORE is only set for the primary thread, this didn't fix
> the original problem for secondary threads. This commit checks
> mm->core_state instead, as already done for /proc/<pid>/status in
> task_core_dumping(). As we have a mm_struct available here anyway, this
> seems to be a clean solution.
>
> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
> ---
>  fs/proc/array.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 2edbb657f859..b76b1e29fc36 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -462,7 +462,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>  		 * a program is not able to use ptrace(2) in that case. It is
>  		 * safe because the task has stopped executing permanently.
>  		 */
> -		if (permitted && (task->flags & PF_DUMPCORE)) {
> +		if (permitted && (!!mm->core_state)) {

This is not entirely safe. mm->core_state is set _before_ zap_process()
is called. Therefore tasks can be executing on a CPU with mm->core_state
set.

With the following additional change, I was able to close the window.

diff --git a/fs/coredump.c b/fs/coredump.c
index e42e17e55bfd..93f55563e2c1 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -340,10 +340,10 @@ static int zap_threads(struct task_struct *tsk, struct mm_struct *mm,
 
 	spin_lock_irq(&tsk->sighand->siglock);
 	if (!signal_group_exit(tsk->signal)) {
-		mm->core_state = core_state;
 		tsk->signal->group_exit_task = tsk;
 		nr = zap_process(tsk, exit_code, 0);
 		clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
+		mm->core_state = core_state;
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
 	if (unlikely(nr < 0))

AFAICT core_state does not need to be set before the other lines. But
there may be some side effects that I overlooked!

John Ogness

Re: [PATCH] proc: report eip and esp for all threads when coredumping

[John Ogness]

Hi Andrew,

On 2019-05-25, Andrew Morton <akpm@linux-foundation.org> wrote:
> Please send along a signed-off-by: for this?

From my response:

On 2019-05-25, John Ogness <john.ogness@linutronix.de> wrote:
> AFAICT core_state does not need to be set before the other lines. But
> there may be some side effects that I overlooked!

The changes I showed were more of a suggestion for Jan than an actual
patch. For a signed-off-by I'll need to do some deeper looking to make
sure what I suggested was safe. Also, we probably need a barrier to make
sure the task flag is cleared before setting core_state. Or instead, we
probably should set core_state after releasing the siglock, setting it
where the PF_DUMPCORE flag is set.

It seems to me that checking for a non-NULL core_state and checking for
the PF_DUMPCORE flag are both used throughout the kernel to identify
core dumps in action. So I think it makes sense to set them "at the same
time". (Or perhaps eliminate PF_DUMPCORE altogether and just use a
non-NULL core_state to identify core dumping.)

I will take a closer look at this.

John Ogness

[PATCHv2] fs/proc: allow reporting eip/esp for all coredumping threads

[John Ogness]

Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
/proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
reintroduced the feature to fix a regression with userspace core dump
handlers (such as minicoredumper).

Because PF_DUMPCORE is only set for the primary thread, this didn't fix
the original problem for secondary threads. Allow reporting the eip/esp
for all threads by checking for PF_EXITING as well. This is set for all
the other threads when they are killed. coredump_wait() waits for all
the tasks to become inactive before proceeding to invoke the core the
core dumper.

Reported-by: Jan Luebbe <jlu@pengutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 This is a rework of Jan's v1 patch that allows accessing eip/esp of all
 the threads without risk of the task still executing on a CPU.

 fs/proc/array.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 2edbb657f859..55180501b915 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -462,7 +462,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		 * a program is not able to use ptrace(2) in that case. It is
 		 * safe because the task has stopped executing permanently.
 		 */
-		if (permitted && (task->flags & PF_DUMPCORE)) {
+		if (permitted && (task->flags & (PF_EXITING|PF_DUMPCORE))) {
 			if (try_get_task_stack(task)) {
 				eip = KSTK_EIP(task);
 				esp = KSTK_ESP(task);
-- 
2.11.0

Re: [PATCHv2] fs/proc: allow reporting eip/esp for all coredumping threads

[Andrew Morton]

On Wed, 29 May 2019 10:55:53 +0200 John Ogness <john.ogness@linutronix.de> wrote:

> Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
> /proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
> ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
> reintroduced the feature to fix a regression with userspace core dump
> handlers (such as minicoredumper).
> 
> Because PF_DUMPCORE is only set for the primary thread, this didn't fix
> the original problem for secondary threads. Allow reporting the eip/esp
> for all threads by checking for PF_EXITING as well. This is set for all
> the other threads when they are killed. coredump_wait() waits for all
> the tasks to become inactive before proceeding to invoke the core the
> core dumper.
> 
> Reported-by: Jan Luebbe <jlu@pengutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  This is a rework of Jan's v1 patch that allows accessing eip/esp of all
>  the threads without risk of the task still executing on a CPU.

Jan's patch ended up including

Fixes: fd7d56270b526ca3 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
Cc: <stable@vger.kernel.org>

Are these not appropriate here?

[PATCHv3] fs/proc: allow reporting eip/esp for all coredumping threads

[John Ogness]

Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
/proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
reintroduced the feature to fix a regression with userspace core dump
handlers (such as minicoredumper).

Because PF_DUMPCORE is only set for the primary thread, this didn't fix
the original problem for secondary threads. Allow reporting the eip/esp
for all threads by checking for PF_EXITING as well. This is set for all
the other threads when they are killed. coredump_wait() waits for all
the tasks to become inactive before proceeding to invoke a core dumper.

Fixes: fd7d56270b526ca3 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
Reported-by: Jan Luebbe <jlu@pengutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 This is a rework of Jan's v1 patch that allows accessing eip/esp of all
 the threads without risk of the task still executing on a CPU.

 The code chagnes are the same as v2. With v3 I included a "Fixes" tag,
 fixed a typo in the commit message, and Cc'd stable.

 fs/proc/array.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 2edbb657f859..55180501b915 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -462,7 +462,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		 * a program is not able to use ptrace(2) in that case. It is
 		 * safe because the task has stopped executing permanently.
 		 */
-		if (permitted && (task->flags & PF_DUMPCORE)) {
+		if (permitted && (task->flags & (PF_EXITING|PF_DUMPCORE))) {
 			if (try_get_task_stack(task)) {
 				eip = KSTK_EIP(task);
 				esp = KSTK_ESP(task);
-- 
2.11.0

Re: [PATCHv3] fs/proc: allow reporting eip/esp for all coredumping threads

[Andrew Morton]

(ooh, Greg, let me do it!)

On Thu, 30 May 2019 02:58:59 +0200 John Ogness <john.ogness@linutronix.de> wrote:

> Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
> /proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
> ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
> reintroduced the feature to fix a regression with userspace core dump
> handlers (such as minicoredumper).
> 
> Because PF_DUMPCORE is only set for the primary thread, this didn't fix
> the original problem for secondary threads. Allow reporting the eip/esp
> for all threads by checking for PF_EXITING as well. This is set for all
> the other threads when they are killed. coredump_wait() waits for all
> the tasks to become inactive before proceeding to invoke a core dumper.
> 
> Fixes: fd7d56270b526ca3 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
> Reported-by: Jan Luebbe <jlu@pengutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  This is a rework of Jan's v1 patch that allows accessing eip/esp of all
>  the threads without risk of the task still executing on a CPU.
> 
>  The code chagnes are the same as v2. With v3 I included a "Fixes" tag,
>  fixed a typo in the commit message, and Cc'd stable.

No, the way to Cc stable is to add

Cc: <stable@vger.kernel.org> to the changelog metadata.

I've made these changes to my copy of this patch, thanks.

Re: [PATCHv3] fs/proc: allow reporting eip/esp for all coredumping threads

[Jan Lübbe]

On Thu, 2019-05-30 at 02:58 +0200, John Ogness wrote:
> Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in
> /proc/PID/stat") stopped reporting eip/esp and commit fd7d56270b52
> ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
> reintroduced the feature to fix a regression with userspace core dump
> handlers (such as minicoredumper).
> 
> Because PF_DUMPCORE is only set for the primary thread, this didn't fix
> the original problem for secondary threads. Allow reporting the eip/esp
> for all threads by checking for PF_EXITING as well. This is set for all
> the other threads when they are killed. coredump_wait() waits for all
> the tasks to become inactive before proceeding to invoke a core dumper.
> 
> Fixes: fd7d56270b526ca3 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping")
> Reported-by: Jan Luebbe <jlu@pengutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Tested-by: Jan Luebbe <jlu@pengutronix.de>

I've tested your patch and can confirm that eip/esp is accessible for
all threads again.

Thanks,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |