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
}
