sysrq: Use SEND_SIG_FORCED instead of force_sig()
PATCH 1/1 sysrq: Use SEND_SIG_FORCED instead of force_sig()
Change send_sig_all() to use do_send_sig_info(SEND_SIG_FORCED) instead of force_sig(SIGKILL). With the recent changes we do not need force_ to kill the CLONE_NEWPID tasks.
And this is more correct. force_sig() can race with the exiting thread, while do_send_sig_info(group => true) kill the whole process.
Some more notes from Oleg Nesterov:
Quoted 9 lines
Just one note. This change makes no difference for
sysrq_handle_kill(). But it obviously changes the behavioursysrq_handle_term(). I think this is fine, if you want to really kill the task which blocks/ignores SIGTERM you can usesysrq_handle_kill().Even ignoring the reasons why
force_sig()is simply wrong here,force_sig(SIGTERM)looks strange. The task won't be killed if it has a handler, butSIG_IGNcan't help. However if it has the handler but blocks SIGTERM temporary (this is very common) it will be killed.
Also,
Quoted 3 lines
force_sig()can't kill the process if the main thread has already exited. IOW, it is trivial to create the process which can't be killed by sysrq.
So, this patch fixes the issue.
---
The patch depends on a few Oleg's patches in -mm, so I believe this should be -mm material as well.
drivers/tty/sysrq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/sysrq.c +1 -1
@@ -329,7 +329,7 @@ static void send_sig_all(int sig)if (is_global_init (p))continue ; -force_sig (sig, p); +do_send_sig_info (sig,SEND_SIG_FORCED , p, true); }read_unlock (&tasklist_lock); }
Signature · 1 line
1.7.9.2
Quoted 45 lines · On Sat, 24 Mar 2012 15:00:24 +0400 Anton Vorontsov…
Change
send_sig_all()to usedo_send_sig_info(SEND_SIG_FORCED)instead offorce_sig(SIGKILL). With the recent changes we do not needforce_to kill theCLONE_NEWPIDtasks.And this is more correct.
force_sig()can race with the exiting thread, whiledo_send_sig_info(group => true)kill the whole process.Some more notes from Oleg Nesterov:
Quoted 9 lines
Just one note. This change makes no difference for
sysrq_handle_kill(). But it obviously changes the behavioursysrq_handle_term(). I think this is fine, if you want to really kill the task which blocks/ignores SIGTERM you can usesysrq_handle_kill().Even ignoring the reasons why
force_sig()is simply wrong here,force_sig(SIGTERM)looks strange. The task won't be killed if it has a handler, butSIG_IGNcan't help. However if it has the handler but blocks SIGTERM temporary (this is very common) it will be killed.Also,
Quoted 3 lines
force_sig()can't kill the process if the main thread has already exited. IOW, it is trivial to create the process which can't be killed by sysrq.So, this patch fixes the issue.
Suggested-by Oleg Nesterov <[email protected]>Acked-by Oleg Nesterov <[email protected]>Signed-off-by Anton Vorontsov <[email protected]>---
The patch depends on a few Oleg's patches in -mm, so I believe this should be -mm material as well.
drivers/tty/sysrq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/tty/sysrq.c +0
-0
drivers/tty/sysrq.c static void send_sig_all(int sig) · line 335
Quoted 8 lines
drivers/tty/sysrq.c +1
-1@@ -329,7 +329,7 @@ static void send_sig_all(int sig)if (is_global_init (p))continue ; -force_sig (sig, p); +do_send_sig_info (sig,SEND_SIG_FORCED , p, true); }read_unlock (&tasklist_lock); }
It's unclear how serious this race is (I'm guessing "not very"), but this patch looks like 3.3 material anyway, yes?
Quoted 1 line · Andrew Morton
On Sat, 24 Mar 2012 15:00:24 +0400
[...]
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
force_sig()can't kill the process if the main thread has already exited. IOW, it is trivial to create the process which can't be killed by sysrq.So, this patch fixes the issue.
[...]
Quoted 2 lines
It's unclear how serious this race is (I'm guessing "not very"), but this patch looks like 3.3 material anyway, yes?
Yep, 3.3 looks like a good target.
Thanks,
Signature · 2 lines
Anton Vorontsov Email: [email protected]
Quoted 7 lines · Andrew Morton
Quoted 7 lines
drivers/tty/sysrq.c +0
-0@@ -329,7 +329,7 @@ static void send_sig_all(int sig)if (is_global_init (p))continue ;
Quoted 7 lines
drivers/tty/sysrq.c static void send_sig_all(int sig) · line 335
Quoted 5 lines
drivers/tty/sysrq.c +1
-1-force_sig (sig, p); +do_send_sig_info (sig,SEND_SIG_FORCED , p, true); }read_unlock (&tasklist_lock); }It's unclear how serious this race is (I'm guessing "not very"),
Well yes, I think that the problems are not very serious.
Quoted 2 lines
but
this patch looks like 3.3 material anyway, yes?
No, this depends on 629d362b9950166c6fac2aa8425db34d824bb043 "signal: give SEND_SIG_FORCED more power to beat SIGNAL_UNKILLABLE".
Oleg.
Quoted 16 lines · On Wed, 28 Mar 2012 22:52:54 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
Quoted 7 lines
drivers/tty/sysrq.c +0
-0@@ -329,7 +329,7 @@ static void send_sig_all(int sig)if (is_global_init (p))continue ;Quoted 7 lines
drivers/tty/sysrq.c static void send_sig_all(int sig) · line 335
Quoted 5 lines
drivers/tty/sysrq.c +1
-1-force_sig (sig, p); +do_send_sig_info (sig,SEND_SIG_FORCED , p, true); }read_unlock (&tasklist_lock); }It's unclear how serious this race is (I'm guessing "not very"),
Quoted 7 lines
Well yes, I think that the problems are not very serious.
Quoted 2 lines
but
this patch looks like 3.3 material anyway, yes?No, this depends on
629d362b9950166c6fac2aa8425db34d824bb043"signal: giveSEND_SIG_FORCEDmore power to beatSIGNAL_UNKILLABLE".
oop, I meant "this patch looks like 3.4 material"?
Quoted 8 lines · Andrew Morton
Quoted 2 lines · On Wed, 28 Mar 2012 22:52:54 +0200 Oleg Nesterov <[email protected]> wrote:
No, this depends on
629d362b9950166c6fac2aa8425db34d824bb043"signal: giveSEND_SIG_FORCEDmore power to beatSIGNAL_UNKILLABLE".oop, I meant "this patch looks like 3.4 material"?
Ah, yes, in this case I agree.
Oleg.