Received-SPF: pass (google.com: domain of linux-kernel+bounces-213484-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 13 Jun 2024 17:00:02 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Rachel Menge <rachelmenge@linux.microsoft.com>,
	linux-kernel@vger.kernel.org, rcu@vger.kernel.org,
	Wei Fu <fuweid89@gmail.com>, apais@linux.microsoft.com,
	Sudhanva Huruli <Sudhanva.Huruli@microsoft.com>,
	Jens Axboe <axboe@kernel.dk>,
	Christian Brauner <brauner@kernel.org>,
	Mike Christie <michael.christie@oracle.com>,
	Joel Granados <j.granados@samsung.com>,
	Mateusz Guzik <mjguzik@gmail.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Frederic Weisbecker <frederic@kernel.org>,
	Neeraj Upadhyay <neeraj.upadhyay@kernel.org>,
	Joel Fernandes <joel@joelfernandes.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Zqiang <qiang.zhang1211@gmail.com>
Subject: Re: [PATCH] zap_pid_ns_processes: don't send SIGKILL to sub-threads
Message-ID: <20240613150001.GB18218@redhat.com>
References: <1386cd49-36d0-4a5c-85e9-bc42056a5a38@linux.microsoft.com>
 <20240608154835.GD7947@redhat.com>
 <87msnpov2z.fsf@email.froward.int.ebiederm.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87msnpov2z.fsf@email.froward.int.ebiederm.org>
User-Agent: Mutt/1.5.24 (2015-08-30)

On 06/13, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.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.