Messages in this thread | | | Date | Mon, 15 Jan 2018 21:51:06 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK() |
| |
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.
| |