linux-kernel - Re: [PATCH] zap_pid_ns_processes: don't send SIGKILL to sub-threads

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]

Message-ID: <87msnpov2z.fsf@email.froward.int.ebiederm.org>
Date: Thu, 13 Jun 2024 08:01:24 -0500
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,  Rachel Menge
 <rachelmenge@...ux.microsoft.com>,  linux-kernel@...r.kernel.org,
  rcu@...r.kernel.org,  Wei Fu <fuweid89@...il.com>,
  apais@...ux.microsoft.com,  Sudhanva Huruli
 <Sudhanva.Huruli@...rosoft.com>,  Jens Axboe <axboe@...nel.dk>,  Christian
 Brauner <brauner@...nel.org>,  Mike Christie
 <michael.christie@...cle.com>,  Joel Granados <j.granados@...sung.com>,
  Mateusz Guzik <mjguzik@...il.com>,  "Paul E. McKenney"
 <paulmck@...nel.org>,  Frederic Weisbecker <frederic@...nel.org>,  Neeraj
 Upadhyay <neeraj.upadhyay@...nel.org>,  Joel Fernandes
 <joel@...lfernandes.org>,  Josh Triplett <josh@...htriplett.org>,  Boqun
 Feng <boqun.feng@...il.com>,  Steven Rostedt <rostedt@...dmis.org>,
  Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,  Lai Jiangshan
 <jiangshanlai@...il.com>,  Zqiang <qiang.zhang1211@...il.com>
Subject: Re: [PATCH] zap_pid_ns_processes: don't send SIGKILL to sub-threads

Oleg Nesterov <oleg@...hat.com> writes:

> The comment above the idr_for_each_entry_continue() loop tries to explain
> why we have to signal each thread in the namespace, but it is outdated.
> This code no longer uses kill_proc_info(), we have a target task so we can
> check thread_group_leader() and avoid the unnecessary group_send_sig_info.
> Better yet, we can change pid_task() to use PIDTYPE_TGID rather than _PID,
> this way it returns NULL if this pid is not a group-leader pid.
>
> Also, change this code to check SIGNAL_GROUP_EXIT, the exiting process /
> thread doesn't necessarily has a pending SIGKILL. Either way these checks
> are racy without siglock, so the patch uses data_race() to shut up KCSAN.

You remove the comment but the meat of what it was trying to say remains
true.  For processes in a session or processes is a process group a list
of all such processes is kept.  No such list is kept for a pid
namespace.  So the best we can do is walk through the allocated pid
numbers in the pid namespace.

It would also help if this explains that in the case of SIGKILL
complete_signal always sets SIGNAL_GROUP_EXIT which makes that a good
check to use to see if the process has been killed (with SIGKILL).
There are races with coredump here but *shrug* I don't think this
changes behavior in that situation.

Eric

> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  kernel/pid_namespace.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index 25f3cf679b35..0f9bd67c9e75 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -191,21 +191,14 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
>  	 * The last thread in the cgroup-init thread group is terminating.
>  	 * Find remaining pid_ts in the namespace, signal and wait for them
>  	 * to exit.
> -	 *
> -	 * Note:  This signals each threads in the namespace - even those that
> -	 * 	  belong to the same thread group, To avoid this, we would have
> -	 * 	  to walk the entire tasklist looking a processes in this
> -	 * 	  namespace, but that could be unnecessarily expensive if the
> -	 * 	  pid namespace has just a few processes. Or we need to
> -	 * 	  maintain a tasklist for each pid namespace.
> -	 *
>  	 */
>  	rcu_read_lock();
>  	read_lock(&tasklist_lock);
>  	nr = 2;
>  	idr_for_each_entry_continue(&pid_ns->idr, pid, nr) {
> -		task = pid_task(pid, PIDTYPE_PID);
> -		if (task && !__fatal_signal_pending(task))
> +		task = pid_task(pid, PIDTYPE_TGID);
> +		/* reading signal->flags is racy without sighand->siglock */
> +		if (task && !(data_race(task->signal->flags) & SIGNAL_GROUP_EXIT))
>  			group_send_sig_info(SIGKILL, SEND_SIG_PRIV, task, PIDTYPE_MAX);
>  	}
>  	read_unlock(&tasklist_lock);