coredump: wait on the core pattern umh at least once
Subsystems (2)
PATCH 1/1 coredump: wait on the core pattern umh at least once
If a thread crashes as a result of a signal on the thread group leader that signal can still be pending, which means the loop in wait_for_dump_helpers() falls straight though.
This can mean that the crashing process is reaped before any core pattern user-mode helper is run, leaving it without entries in /proc to look through.
While the helper obviously has to deal with that, tweaking this loop so it runs at least one iteration even in that case helps a lot.
--- fs/exec.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
fs/exec.c +2 -2
@@ -2030,11 +2030,11 @@ static void wait_for_dump_helpers(struct file *file) pipe->readers++; pipe->writers--; -while ((pipe->readers >1 ) && (!signal_pending (current))) { +do {wake_up_interruptible_sync (&pipe->wait);kill_fasync (&pipe->fasync_readers,SIGIO ,POLL_IN );pipe_wait (pipe); - } + }while ((pipe->readers >1 ) && (!signal_pending (current))); pipe->readers--; pipe->writers++;
Signature · 1 line
1.7.3.1
Quoted 38 lines · Scott James Remnant
If a thread crashes as a result of a signal on the thread group leader that signal can still be pending, which means the loop in
wait_for_dump_helpers()falls straight though.This can mean that the crashing process is reaped before any core pattern user-mode helper is run, leaving it without entries in /proc to look through.
While the helper obviously has to deal with that, tweaking this loop so it runs at least one iteration even in that case helps a lot.
Signed-off-by Scott James Remnant <[email protected]>--- fs/exec.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)fs/exec.c +2
-2@@ -2030,11 +2030,11 @@ static void wait_for_dump_helpers(struct file *file) pipe->readers++; pipe->writers--; -while ((pipe->readers >1 ) && (!signal_pending (current))) { +do {wake_up_interruptible_sync (&pipe->wait);kill_fasync (&pipe->fasync_readers,SIGIO ,POLL_IN );pipe_wait (pipe); - } + }while ((pipe->readers >1 ) && (!signal_pending (current))); pipe->readers--; pipe->writers++;Signature · 1 line
1.7.3.1
Seems reasonable to me.
Quoted 3 lines · Neil Horman
Seems reasonable to me.
Acked-by Neil Horman <[email protected]>
One thing I'm not sure about is whether this will work if there are actually no readers really reading, just the incremented one. It seemed to work just fine in testing, but I'd be more comfortable if someone else could confirm.
Scott
Quoted 3 lines · Scott James Remnant
If a thread crashes as a result of a signal on the thread group leader that signal can still be pending,
No. do_coredump() clears TIF_SIGPENDING.
The problem is, this is obviously not enough and should be fixed.
Quoted 2 lines
While the helper obviously has to deal with that, tweaking this loop so it runs at least one iteration even in that case helps a lot.
I don't understand this patch. It doesn't look right at all.
fs/exec.c static void wait_for_dump_helpers(struct file *file) · line 2036
Quoted 9 lines
fs/exec.c +1
-1@@ -2030,11 +2030,11 @@ static void wait_for_dump_helpers(struct file *file) pipe->readers++; pipe->writers--; -while ((pipe->readers >1 ) && (!signal_pending (current))) { + do { wake_up_interruptible_sync(&pipe->wait); kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); pipe_wait(pipe);
This can hang forever. We didn't check pipe->readers, it it is zero nobody can wakeup us.
fs/exec.c static void wait_for_dump_helpers(struct file *file) · line 2037
Quoted 1 line
fs/exec.c +1
-0+ }while ((pipe->readers >1 ) && (!signal_pending (current)));
And, it doesn't make any sense to call pipe_wait() with signal_pending(), it won't block. Note that pipe_wait() schedules in TASK_INTERRUPTIBLE.
I already tried to explain why this signal_pending() was added, but apparently I was not clear. I'll try again in the previous thread.
Oleg.
Quoted 7 lines · Oleg Nesterov
Quoted 3 lines · Scott James Remnant
If a thread crashes as a result of a signal on the thread group leader that signal can still be pending,
No.
do_coredump()clearsTIF_SIGPENDING.
I'm definitely seeing cases where SIGTERM sent to the process group that chrome is in results in one of chrome's thread's crashing (not your concern, obviously), but at the point it enters this function TIF_SIGPENDING is definitely set and the signal is SIGTERM.
The SIGTERM is in the shared pending set.
Quoted 3 lines
I already tried to explain why this
signal_pending()was added, but apparently I was not clear. I'll try again in the previous thread.
Could you add me to the Cc: of that thread?
Scott
Quoted 5 lines · Scott James Remnant
Quoted 2 lines · Oleg Nesterov
Quoted 1 line · Scott James Remnant
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
If a thread crashes as a result of a signal on the thread group leader that signal can still be pending,
No.
do_coredump()clearsTIF_SIGPENDING.I'm definitely seeing cases where SIGTERM sent to the process group that chrome is in results in one of chrome's thread's crashing (not your concern, obviously), but at the point it enters this function
which function? wait_for_dump_helpers?
Quoted 1 line
TIF_SIGPENDINGis definitely set and the signal is SIGTERM.
Yes, this is possible. But not as result of a signal which triggers the coredumping. And once again, this clear_thread_flag(TIF_SIGPENDING) is simply wrong (I mean, not enough).
Quoted 4 lines
Quoted 3 lines
I already tried to explain why this
signal_pending()was added, but apparently I was not clear. I'll try again in the previous thread.Could you add me to the Cc: of that thread?
I thought you were cc'ed ;) Sorry, I didn't realiaze that these 2 threads are totally separate. Please look at
http://marc.info/?t=131959137800005
and at
http://marc.info/?l=linux-kernel&m=131989970411759
in particular.
Oleg.
Quoted 15 lines · Oleg Nesterov
Quoted 5 lines · Scott James Remnant
Quoted 2 lines · Oleg Nesterov
Quoted 1 line · Scott James Remnant
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
If a thread crashes as a result of a signal on the thread group leader that signal can still be pending,
No.
do_coredump()clearsTIF_SIGPENDING.I'm definitely seeing cases where SIGTERM sent to the process group that chrome is in results in one of chrome's thread's crashing (not your concern, obviously), but at the point it enters this function
Quoted 8 lines
which function?
wait_for_dump_helpers?Quoted 1 line
TIF_SIGPENDINGis definitely set and the signal is SIGTERM.Yes, this is possible. But not as result of a signal which triggers the coredumping. And once again, this
clear_thread_flag(TIF_SIGPENDING)is simply wrong (I mean, not enough).
Makes sense.
Quoted 8 lines
Quoted 4 lines
Quoted 3 lines
I already tried to explain why this
signal_pending()was added, but apparently I was not clear. I'll try again in the previous thread.Could you add me to the Cc: of that thread?
I thought you were cc'ed ;) Sorry, I didn't realiaze that these 2 threads are totally separate. Please look at
Quoted 7 lines
http://marc.info/?t=131959137800005
and at
http://marc.info/?l=linux-kernel&m=131989970411759in particular.
Yeah I couldn't really work out why the signal_pending() was there either, I was trying to rework the loop to keep it on the assumption it was there for a reason - I'm quite happy that it get removed, that also fixes my problem :)
Scott
Quoted 14 lines · Scott James Remnant
Quoted 7 lines · Oleg Nesterov
Quoted 3 lines · Scott James Remnant
If a thread crashes as a result of a signal on the thread group leader that signal can still be pending,
No.
do_coredump()clearsTIF_SIGPENDING.I'm definitely seeing cases where SIGTERM sent to the process group that chrome is in results in one of chrome's thread's crashing (not your concern, obviously), but at the point it enters this function
TIF_SIGPENDINGis definitely set and the signal is SIGTERM.
Quoted 9 lines
The SIGTERM is in the shared pending set.
Quoted 3 lines
I already tried to explain why this
signal_pending()was added, but apparently I was not clear. I'll try again in the previous thread.Could you add me to the Cc: of that thread?
Scott
FWIW, this is the (huge) thread, and specific post that originated the change we're looking at:
https://lkml.org/lkml/2009/7/2/98
Neil