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: <20240613150001.GB18218@redhat.com>
Date: Thu, 13 Jun 2024 17:00:02 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: "Eric W. Biederman" <ebiederm@...ssion.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

On 06/13, Eric W. Biederman wrote:
>
> 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.

OK, I'll recheck tomorrow. Yet I think it doesn't make sense to send
SIGKILL to sub-threads, and the comment looks misleading today. This was
the main motivation, but again, I'll recheck.

> 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).

Well, if SIGNAL_GROUP_EXIT is set we do not care if this process was
killed or not. It (the whole thread group) is going to exit, that is all.

We can even remove this check, it is just the optimization, just like
the current fatal_signal_pending() check.

Oleg.