[PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Anton Vorontsov]

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

Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Andrew Morton]

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?

Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Anton Vorontsov]

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

Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Oleg Nesterov]

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.

Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Andrew Morton]

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"?

Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Oleg Nesterov]

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.

[PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Anton Vorontsov]

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

Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Greg Kroah-Hartman]

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>

Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig()

[Oleg Nesterov]

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>