fix reading ESP during coredump

Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in /proc/PID/stat") disabled stack pointer reading, because it is generally dangerous to do so.

Commit fd7d56270b52 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping") made an exception for coredumping thread, because for this case it is safe.

The exception was later extended to all threads in a coredumping process by commit cb8f381f1613 ("fs/proc/array.c: allow reporting eip/esp for all coredumping threads").

The above two commits determine if a task is core dumping by checking the PF_EXITING and PF_DUMPCORE flags.

However, commit 92307383082d ("coredump: Don't perform any cleanups before dumping core") moved coredump to happen earlier and before PF_EXITING is set. Thus, the check of the PF_EXITING flag no longer works.

Instead, use task->signal->core_state to determine if coredump is happening. This pointer is set at the beginning of coredump and is cleared once coredump is done. Thus, while this pointer is not NULL, it is safe to read ESP.

Fixes 92307383082d ("coredump: Don't perform any cleanups before dumping core")

Signed-off-by Nam Cao <[email protected]>

Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in /proc/PID/stat") disabled stack pointer reading, because it is generally dangerous to do so.

Commit fd7d56270b52 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping") made an exception for coredumping thread, because for this case it is safe.

The exception was later extended to all threads in a coredumping process by commit cb8f381f1613 ("fs/proc/array.c: allow reporting eip/esp for all coredumping threads").

The above two commits determine if a task is core dumping by checking the PF_EXITING and PF_DUMPCORE flags.

However, commit 92307383082d ("coredump: Don't perform any cleanups before dumping core") moved coredump to happen earlier and before PF_EXITING is set. Thus, the check of the PF_EXITING flag no longer works.

Instead, use task->signal->core_state to determine if coredump is happening. This pointer is set at the beginning of coredump and is cleared once coredump is done. Thus, while this pointer is not NULL, it is safe to read ESP.

Fixes 92307383082d ("coredump: Don't perform any cleanups before dumping core")

Review static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 523

Quoted 9 lines · Nam Cao

(quoted diff) +4 -0

@@ -534,6 +517,23 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		ppid = task_tgid_nr_ns(task->real_parent, ns);
 		pgid = task_pgrp_nr_ns(task, ns);
 
+		/*
+		 * esp and eip are intentionally zeroed out.  There is no
+		 * non-racy way to read them without freezing the task.
+		 * Programs that need reliable values can use ptrace(2).

OK,

but then:

Review static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 532

Quoted 9 lines

(quoted diff) +9 -0

+		 * The only exception is if the task is core dumping because
+		 * 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->signal->core_state) {
+			if (try_get_task_stack(task)) {
+				eip = KSTK_EIP(task);
+				esp = KSTK_ESP(task);
+				put_task_stack(task);

How can the task->signal->core_state check help ?

Suppose we have a task T1 with T1-pid == 100 and you read /proc/100/stat. It is possible that the T1's sub-thread T2 starts the coredumping and sets signal->core_state != NULL.

But read(/proc/100/stat) can run before T1 gets SIGKILL from T2 and enters the kernel mode?

Quoted 28 lines · Oleg Nesterov

Review static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 523

Quoted 9 lines · Nam Cao

(quoted diff) +4 -0

@@ -534,6 +517,23 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		ppid = task_tgid_nr_ns(task->real_parent, ns);
 		pgid = task_pgrp_nr_ns(task, ns);
 
+		/*
+		 * esp and eip are intentionally zeroed out.  There is no
+		 * non-racy way to read them without freezing the task.
+		 * Programs that need reliable values can use ptrace(2).

OK,

but then:

Review static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 532

Quoted 9 lines

(quoted diff) +9 -0

+		 * The only exception is if the task is core dumping because
+		 * 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->signal->core_state) {
+			if (try_get_task_stack(task)) {
+				eip = KSTK_EIP(task);
+				esp = KSTK_ESP(task);
+				put_task_stack(task);

How can the task->signal->core_state check help ?

Quoted 6 lines

Suppose we have a task T1 with T1-pid == 100 and you read /proc/100/stat. It is possible that the T1's sub-thread T2 starts the coredumping and sets signal->core_state != NULL.

But read(/proc/100/stat) can run before T1 gets SIGKILL from T2 and enters the kernel mode?

Can't the trivial patch below fix the problem?

Oleg.

fs/proc/array.c renamed from proc/array.c +1 -1

@@ -500,7 +500,7 @@
 		 * 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_EXITING|PF_DUMPCORE))) {
+		if (permitted && (task->flags & (PF_EXITING|PF_DUMPCORE|PF_POSTCOREDUMP))) {
 			if (try_get_task_stack(task)) {
 				eip = KSTK_EIP(task);
 				esp = KSTK_ESP(task);

Quoted 2 lines

Can't the trivial patch below fix the problem?

It can. In fact this is the original fix we had. I thought that checking a single "core_state" is simpler than checking 3 flags, oh well..

Can you send a proper patch, or should I do it?

Add a test which checks that the kstkesp field in /proc/pid/stat can be read for all threads of a coredumping process.

For full details including the motivation for this test and how it works, see the README file added by this commit.

Signed-off-by Nam Cao <[email protected]>