Messages in this thread Patch in this message | | | Date | Fri, 12 Jan 2018 17:42:34 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK() |
| |
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.
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?
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?
Oleg.
--- 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 }
| |