* [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
@ 2012-03-24 11:00 Anton Vorontsov
2012-03-26 22:43 ` Andrew Morton
0 siblings, 1 reply; 9+ messages in thread
From: Anton Vorontsov @ 2012-03-24 11:00 UTC (permalink / raw)
To: Andrew Morton; +Cc: Greg Kroah-Hartman, Oleg Nesterov, Alan Cox, linux-kernel
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:
> Just one note. This change makes no difference for sysrq_handle_kill().
> But it obviously changes the behaviour sysrq_handle_term(). I think
> this is fine, if you want to really kill the task which blocks/ignores
> SIGTERM you can use sysrq_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, but SIG_IGN can't help. However if it has the handler
> but blocks SIGTERM temporary (this is very common) it will be killed.
Also,
> 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 <oleg@redhat.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
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(-)
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 8db9125..5ab8039 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -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);
}
--
1.7.9.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
2012-03-24 11:00 [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig() Anton Vorontsov
@ 2012-03-26 22:43 ` Andrew Morton
2012-03-27 13:03 ` Anton Vorontsov
2012-03-28 20:52 ` Oleg Nesterov
0 siblings, 2 replies; 9+ messages in thread
From: Andrew Morton @ 2012-03-26 22:43 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: Greg Kroah-Hartman, Oleg Nesterov, Alan Cox, linux-kernel
On Sat, 24 Mar 2012 15:00:24 +0400
Anton Vorontsov <anton.vorontsov@linaro.org> wrote:
> 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:
>
> > Just one note. This change makes no difference for sysrq_handle_kill().
> > But it obviously changes the behaviour sysrq_handle_term(). I think
> > this is fine, if you want to really kill the task which blocks/ignores
> > SIGTERM you can use sysrq_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, but SIG_IGN can't help. However if it has the handler
> > but blocks SIGTERM temporary (this is very common) it will be killed.
>
> Also,
>
> > 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 <oleg@redhat.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
> ---
>
> 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(-)
>
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 8db9125..5ab8039 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
2012-03-26 22:43 ` Andrew Morton
@ 2012-03-27 13:03 ` Anton Vorontsov
2012-03-28 20:52 ` Oleg Nesterov
1 sibling, 0 replies; 9+ messages in thread
From: Anton Vorontsov @ 2012-03-27 13:03 UTC (permalink / raw)
To: Andrew Morton; +Cc: Greg Kroah-Hartman, Oleg Nesterov, Alan Cox, linux-kernel
On Mon, Mar 26, 2012 at 03:43:03PM -0700, Andrew Morton wrote:
> On Sat, 24 Mar 2012 15:00:24 +0400
[...]
> > > 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.
[...]
> 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,
--
Anton Vorontsov
Email: cbouatmailru@gmail.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
2012-03-26 22:43 ` Andrew Morton
2012-03-27 13:03 ` Anton Vorontsov
@ 2012-03-28 20:52 ` Oleg Nesterov
2012-03-28 21:08 ` Andrew Morton
1 sibling, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2012-03-28 20:52 UTC (permalink / raw)
To: Andrew Morton; +Cc: Anton Vorontsov, Greg Kroah-Hartman, Alan Cox, linux-kernel
On 03/26, Andrew Morton wrote:
>
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -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"),
Well yes, I think that the problems are not very serious.
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
2012-03-28 20:52 ` Oleg Nesterov
@ 2012-03-28 21:08 ` Andrew Morton
2012-03-28 21:26 ` Oleg Nesterov
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2012-03-28 21:08 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Anton Vorontsov, Greg Kroah-Hartman, Alan Cox, linux-kernel
On Wed, 28 Mar 2012 22:52:54 +0200
Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/26, Andrew Morton wrote:
> >
> > > --- a/drivers/tty/sysrq.c
> > > +++ b/drivers/tty/sysrq.c
> > > @@ -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"),
>
> Well yes, I think that the problems are not very serious.
>
> > 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".
oop, I meant "this patch looks like 3.4 material"?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
2012-03-28 21:08 ` Andrew Morton
@ 2012-03-28 21:26 ` Oleg Nesterov
0 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2012-03-28 21:26 UTC (permalink / raw)
To: Andrew Morton; +Cc: Anton Vorontsov, Greg Kroah-Hartman, Alan Cox, linux-kernel
On 03/28, Andrew Morton wrote:
>
> On Wed, 28 Mar 2012 22:52:54 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > No, this depends on 629d362b9950166c6fac2aa8425db34d824bb043
> > "signal: give SEND_SIG_FORCED more power to beat SIGNAL_UNKILLABLE".
>
> oop, I meant "this patch looks like 3.4 material"?
Ah, yes, in this case I agree.
Oleg.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
2012-02-14 22:50 ` Anton Vorontsov
2012-02-14 23:03 ` Greg Kroah-Hartman
@ 2012-02-15 13:53 ` Oleg Nesterov
1 sibling, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2012-02-15 13:53 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: Andrew Morton, Greg Kroah-Hartman, rientjes, linux-kernel
On 02/15, Anton Vorontsov wrote:
>
> 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.
ACK.
Just one note. This change makes no difference for sysrq_handle_kill().
But it obviously changes the behaviour sysrq_handle_term(). I think
this is fine, if you want to really kill the task which blocks/ignores
SIGTERM you can use sysrq_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, but SIG_IGN can't help. However if it has the handler
but blocks SIGTERM temporary (this is very common) it will be killed.
> And this is more correct. force_sig() can race with the exiting
> thread, while do_send_sig_info(group => true) kill the whole
> process.
Yes, except the word "race" doesn't look accurate. 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.
> > > @@ -324,9 +324,12 @@ static void send_sig_all(int sig)
> > >
> > > read_lock(&tasklist_lock);
> > > for_each_process(p) {
> > > - if (p->mm && !is_global_init(p))
> > > - /* Not swapper, init nor kernel thread */
> > > - force_sig(sig, p);
> > > + if (p->flags & PF_KTHREAD)
> > > + continue;
> > > + if (is_global_init(p))
> > > + continue;
> > > +
> > > + force_sig(sig, p);
> > > }
> > > read_unlock(&tasklist_lock);
> >
> > Obviously I agree with this change.
> >
> > But where does this read_lock(tasklist) come from?
>
> It came from this patch: http://lkml.org/lkml/2012/2/7/24
>
> > We discussed this with Anton. Yes, tasklist ensures that
> > force_sig() can't crash the kernel. But it is still wrong
> > and should not be used.
> >
> > I think send_sig_all() should use SEND_SIG_FORCED (this
> > depends on the patches I sent to Andrew), in this case
> > tasklist is not needed.
>
> Well, I think the lock is still a good thing: we don't want
> any new processes to be created while we kill others.
Yes, but
> I might be wrong, but copy_process() issues recalc_sigpending()
> under tasklist lock especially the for this scenario.
note that it checks recalc_sigpending() under ->siglock. This
means copy_process() can't race with do_send_sig_info() which
takes the same lock. Either the forking task should see
TIF_SIGPENDING, or send_sig_all() should see the result of
list_add_tail_rcu(&p->tasks, &init_task.tasks).
However, we can race with exec. This needs the trivial fix, but:
> Sysrq is a rare thing, so there is actually should be no problem
> with holding the lock.
OK. This looks simpler.
> So, how about this patch?
>
> Greg, can we take it via -mm tree, as it depends on a few
> sched patches?
>
> drivers/tty/sysrq.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 8db9125..5ab8039 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -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);
Acked-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
2012-02-14 22:50 ` Anton Vorontsov
@ 2012-02-14 23:03 ` Greg Kroah-Hartman
2012-02-15 13:53 ` Oleg Nesterov
1 sibling, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2012-02-14 23:03 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: Oleg Nesterov, Andrew Morton, rientjes, linux-kernel
On Wed, Feb 15, 2012 at 02:50:17AM +0400, Anton Vorontsov wrote:
> 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.
>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
> ---
>
> On Fri, Feb 10, 2012 at 09:10:08PM +0100, Oleg Nesterov wrote:
> > > --- a/drivers/tty/sysrq.c
> > > +++ b/drivers/tty/sysrq.c
> > > @@ -324,9 +324,12 @@ static void send_sig_all(int sig)
> > >
> > > read_lock(&tasklist_lock);
> > > for_each_process(p) {
> > > - if (p->mm && !is_global_init(p))
> > > - /* Not swapper, init nor kernel thread */
> > > - force_sig(sig, p);
> > > + if (p->flags & PF_KTHREAD)
> > > + continue;
> > > + if (is_global_init(p))
> > > + continue;
> > > +
> > > + force_sig(sig, p);
> > > }
> > > read_unlock(&tasklist_lock);
> >
> > Obviously I agree with this change.
> >
> > But where does this read_lock(tasklist) come from?
>
> It came from this patch: http://lkml.org/lkml/2012/2/7/24
>
> > We discussed this with Anton. Yes, tasklist ensures that
> > force_sig() can't crash the kernel. But it is still wrong
> > and should not be used.
> >
> > I think send_sig_all() should use SEND_SIG_FORCED (this
> > depends on the patches I sent to Andrew), in this case
> > tasklist is not needed.
>
> Well, I think the lock is still a good thing: we don't want
> any new processes to be created while we kill others.
>
> I might be wrong, but copy_process() issues recalc_sigpending()
> under tasklist lock especially the for this scenario.
>
> So, in this and in OOM cases we have to be precise (unlike LMK).
> Sysrq is a rare thing, so there is actually should be no problem
> with holding the lock.
>
> So, how about this patch?
>
> Greg, can we take it via -mm tree, as it depends on a few
> sched patches?
That's fine with me:
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()
[not found] ` <20120210201008.GA21009@redhat.com>
@ 2012-02-14 22:50 ` Anton Vorontsov
2012-02-14 23:03 ` Greg Kroah-Hartman
2012-02-15 13:53 ` Oleg Nesterov
0 siblings, 2 replies; 9+ messages in thread
From: Anton Vorontsov @ 2012-02-14 22:50 UTC (permalink / raw)
To: Oleg Nesterov, Andrew Morton, Greg Kroah-Hartman; +Cc: rientjes, linux-kernel
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.
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
On Fri, Feb 10, 2012 at 09:10:08PM +0100, Oleg Nesterov wrote:
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -324,9 +324,12 @@ static void send_sig_all(int sig)
> >
> > read_lock(&tasklist_lock);
> > for_each_process(p) {
> > - if (p->mm && !is_global_init(p))
> > - /* Not swapper, init nor kernel thread */
> > - force_sig(sig, p);
> > + if (p->flags & PF_KTHREAD)
> > + continue;
> > + if (is_global_init(p))
> > + continue;
> > +
> > + force_sig(sig, p);
> > }
> > read_unlock(&tasklist_lock);
>
> Obviously I agree with this change.
>
> But where does this read_lock(tasklist) come from?
It came from this patch: http://lkml.org/lkml/2012/2/7/24
> We discussed this with Anton. Yes, tasklist ensures that
> force_sig() can't crash the kernel. But it is still wrong
> and should not be used.
>
> I think send_sig_all() should use SEND_SIG_FORCED (this
> depends on the patches I sent to Andrew), in this case
> tasklist is not needed.
Well, I think the lock is still a good thing: we don't want
any new processes to be created while we kill others.
I might be wrong, but copy_process() issues recalc_sigpending()
under tasklist lock especially the for this scenario.
So, in this and in OOM cases we have to be precise (unlike LMK).
Sysrq is a rare thing, so there is actually should be no problem
with holding the lock.
So, how about this patch?
Greg, can we take it via -mm tree, as it depends on a few
sched patches?
drivers/tty/sysrq.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 8db9125..5ab8039 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -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);
}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-03-28 21:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-24 11:00 [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig() Anton Vorontsov
2012-03-26 22:43 ` Andrew Morton
2012-03-27 13:03 ` Anton Vorontsov
2012-03-28 20:52 ` Oleg Nesterov
2012-03-28 21:08 ` Andrew Morton
2012-03-28 21:26 ` Oleg Nesterov
[not found] <13288070803232@kroah.org>
[not found] ` <20120210201008.GA21009@redhat.com>
2012-02-14 22:50 ` Anton Vorontsov
2012-02-14 23:03 ` Greg Kroah-Hartman
2012-02-15 13:53 ` Oleg Nesterov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.