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 <[email protected]>
---
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
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.
On Wed, 22 May 2019 18:16:14 +0200 Jan Luebbe <[email protected]> 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.
On Wed, 2019-05-22 at 11:00 -0700, Andrew Morton wrote:
> On Wed, 22 May 2019 18:16:14 +0200 Jan Luebbe <[email protected]> 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 |
On 2019-05-22, Jan Luebbe <[email protected]> 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 <[email protected]>
> ---
> 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
Hi Andrew,
On 2019-05-25, Andrew Morton <[email protected]> wrote:
> Please send along a signed-off-by: for this?
From my response:
On 2019-05-25, John Ogness <[email protected]> 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