Messages in this thread | | | Date | Wed, 15 Feb 2012 14:53:06 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig() |
| |
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>
| |