On 12.01.2018 19:42, Oleg Nesterov wrote:
> On 01/12, Kirill Tkhai wrote:
>>
>> How about this patch instead of the whole set? I left thread iterations
>> and added sighand locking for visability.
>
> Kirill, I didn't really read this series so I don't quite understand what
> are you actually trying to do...
>
> __do_SAK() is racy anyway, a process can open tty right after it was checked,
> and I do not understand why should we care about races with execve.
Please, just ignore two first patches. As I wrote I thought we iterate threads
to close race with exec (and missed that threads may have unshared fd table).
So, if we didn't use to care about such situations, I don't care them now. My
main
target is just to speed up __do_SAK().
> 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.
> And whatever we do, I think you are right and for_each_process() makes more
> sense, and in the likely case all sub-threads should share the same
> file_struct.
> So perhaps we should start with the simple cleanup? Say,
>
> for_each_process(p) {
> if (p->signal->tty == tty) {
> tty_notice(tty, "SAK: killed process %d (%s): by
> controlling tty\n",
> task_pid_nr(p), p->comm);
> goto kill;
> }
>
> files = NULL;
> for_each_thread(p, t) {
> if (t->files == files) /* racy but we do not
> care */
> continue;
>
> task_lock(t);
> files = t->files;
> i = iterate_fd(files, 0, this_tty, tty);
> task_unlock(t);
>
> if (i != 0) {
> tty_notice(tty, "SAK: killed process %d
> (%s): by fd#%d\n",
> task_pid_nr(p), p->comm, i -
> 1);
> goto kill;
> }
> }
>
> continue;
> kill:
> force_sig(SIGKILL, p);
> }
>
> (see the uncompiled/untested patch below), then make another change to avoid
> tasklist_lock?
>
>
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2704,7 +2704,8 @@ void __do_SAK(struct tty_struct *tty)
> #ifdef TTY_SOFT_SAK
> tty_hangup(tty);
> #else
> - struct task_struct *g, *p;
> + struct task_struct *p, *t;
> + struct files_struct files;
> struct pid *session;
> int i;
>
> @@ -2725,22 +2726,34 @@ void __do_SAK(struct tty_struct *tty)
> } while_each_pid_task(session, PIDTYPE_SID, p);
>
> /* Now kill any processes that happen to have the tty open */
> - do_each_thread(g, p) {
> + for_each_process(p) {
> if (p->signal->tty == tty) {
> tty_notice(tty, "SAK: killed process %d (%s): by
> controlling tty\n",
> task_pid_nr(p), p->comm);
> - send_sig(SIGKILL, p, 1);
> - continue;
> + goto kill;
> }
> - task_lock(p);
> - i = iterate_fd(p->files, 0, this_tty, 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);
> +
> + files = NULL;
> + for_each_thread(p, t) {
> + if (t->files == files) /* racy but we do not
> care */
> + continue;
> +
> + task_lock(t);
> + files = t->files;
> + i = iterate_fd(files, 0, this_tty, tty);
> + task_unlock(t);
> +
> + if (i != 0) {
> + tty_notice(tty, "SAK: killed process %d
> (%s): by fd#%d\n",
> + task_pid_nr(p), p->comm, i -
> 1);
> + goto kill;
> + }
> }
> - task_unlock(p);
> - } while_each_thread(g, p);
> +
> + continue;
> +kill:
> + force_sig(SIGKILL, p);
> + }
> read_unlock(&tasklist_lock);
> #endif
> }
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?
Also, the below patch will go on top of yours:
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 979eb5d80fe9..20b74b4c9f84 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2724,7 +2724,9 @@ void __do_SAK(struct tty_struct *tty)
task_pid_nr(p), p->comm);
send_sig(SIGKILL, p, 1);
} while_each_pid_task(session, PIDTYPE_SID, p);
+ read_unlock(&tasklist_lock);
+ rcu_read_lock();
/* Now kill any processes that happen to have the tty open */
for_each_process(p) {
if (p->signal->tty == tty) {
@@ -2752,9 +2754,9 @@ void __do_SAK(struct tty_struct *tty)
continue;
kill:
- force_sig(SIGKILL, p);
+ send_sig(SIGKILL, p, 1);
}
- read_unlock(&tasklist_lock);
+ rcu_read_unlock();
#endif
}
I replaced force_sig() as it does not check for task's sighand.
Kirill
