Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK()

[Oleg Nesterov]

On 01/15, Kirill Tkhai wrote:
>
> On 12.01.2018 19:42, Oleg Nesterov wrote:
>
> > IOW, I do not understand why we can't simply use rcu_read_lock() after
> > do_each_pid_task/while_each_pid_task. Yes we can miss the new process/thread,
> > but if the creator process had this tty opened it should be killed by us so
> > fork/clone can't succeed: both do_fork() and send_sig() take the same lock
> > and do_fork() checks signal_pending() under ->siglock.
> >
> > No?
>
> Yes, but we send signal not every time. So, this was the only reason I added
> lock/unlock the locks. But anyway, __do_SAK() is racy and the effect of that
> is minimal, so it seems we may skip this.

Yes. If we don't send SIGKILL we do not care about the new child process/thread
we can miss, it can't have this tty opened at fork() time. If the child opens
this tty after that, __do_SAK can "miss" it anyway in that it can see it before
it does open(tty).

> I tested your patch with small modification in "struct files_struct *files;" ('*' is added).
> Could I send it with your "Signed-off-by" as the second version?

Yes, please feel free,

>  kill:
> -		force_sig(SIGKILL, p);
> +		send_sig(SIGKILL, p, 1);

Agreed, I didn't actually want to use force_sig(SIGKILL), copy-and-paste error.

But. on the second thought this probably needs another change... I don't understand
these force_sig/send_sig in __do_SAK().

If signal->tty == tty it does send_sig(SIGKILL), this won't kill the global or
sub-namespace init.

However, if iterate_fd() finds this tty it does force_sig(SIGKILL) which clears
SIGNAL_UNKILLABLE, so it can kill even the global init.

This looks strange, and probably unintentional. So it seems yoou should start
with "revert 20ac94378 [PATCH] do_SAK: Don't recursively take the tasklist_lock" ?
The original reason for that commit has gone a long ago.

At the same time, I do not know if we actually want to kill sub-namespace inits
or not. If yes, we can use SEND_SIG_FORCED (better than ugly force_sig()) but
skip the global init. But this will need yet another change.

Oleg.