Re: [PATCH v2 1/3] Revert "do_SAK: Don't recursively take the tasklist_lock"

[Oleg Nesterov]

On 01/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@xxxxxxxxxx> writes:
>
> > On 01/17, Eric W. Biederman wrote:
> >
> >> Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> writes:
> >>
> >> > This reverts commit 20ac94378de5.
> >> >
> >> > send_sig() does not take tasklist_lock for a long time,
> >> > so this commit and the problem it solves are not relevant
> >> > anymore.
> >> >
> >> > Also, the problem of force_sig() is it clears SIGNAL_UNKILLABLE
> >> > flag, thus even global init may be killed by __do_SAK(),
> >> > which is definitely not the expected behavior.
> >>
> >> Actually it is.
> >>
> >> SAK should kill everything that has the tty open.  If init opens the tty
> >> I am so sorry, it can not operate correctly.  init should not have your
> >> tty open.
> >
> > OK, but then we need "force" in other places too. __do_SAK() does send_sig(SIGKILL)
> > in do_each_pid_task(PIDTYPE_SID) and if signal->tty == tty.
> >
> > Plus force_sig() is not rcu-friendly.
> >
> > So I personally agree with this change. Whether we want to kill the global init
> > or not should be discussed, if we want to do this __do_SAK() should use
> > SEND_SIG_FORCED and this is what Kirill is going to do (iiuc), but this needs
> > another patch.
>
> To operate correctly, do_SAK() needs to kill everything that has the tty
> open.  Unless we can make that guarantee I don't see the point of
> changing do_SAK.

OK, but how this connects to this change?

Again, this force_sig() doesn't match other send_sig()'s in __do_SAK(),
and Kirill is going to turn them all into send_sig_info(SEND_SIG_FORCED).
Just we need to discuss whether we need to skip the global init or not
but this is another story.

So why do you dislike this change?

force_sig() should die anyway. At least in its current form, it should not
be used unless task == current. But this is off-topic.

Oleg.