linux-kernel - Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 15 Jan 2018 12:32:22 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        jslaby@...e.com, viro@...iv.linux.org.uk, keescook@...omium.org,
        serge@...lyn.com, james.l.morris@...cle.com, luto@...nel.org,
        john.johansen@...onical.com, mingo@...nel.org,
        akpm@...ux-foundation.org, mhocko@...e.com, peterz@...radead.org
Subject: Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK()

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