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

[ebiederm@xmission.com (Eric W. Biederman)]

Kirill Tkhai <ktkhai@virtuozzo.com> 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.

The alternative and perhaps the better option is to simply remove SAK
support, if we can not make it race free.

But yes SAK very much does care about races.  

> Came from discussion in "tty: Iterate only thread group leaders in __do_SAK()"
> https://lkml.org/lkml/2018/1/11/492
>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  drivers/tty/tty_io.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index dc60aeea87d8..84715ba1aee2 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2737,7 +2737,7 @@ void __do_SAK(struct tty_struct *tty)
>  		if (i != 0) {
>  			tty_notice(tty, "SAK: killed process %d (%s): by fd#%d\n",
>  				   task_pid_nr(p), p->comm, i - 1);
> -			force_sig(SIGKILL, p);
> +			send_sig(SIGKILL, p, 1);
>  		}
>  		task_unlock(p);
>  	} while_each_thread(g, p);