Re: [PATCH] pidfd: prevent creation of pidfds for kthreads

[Christian Brauner <brauner@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

On Sat, Aug 17, 2024 at 08:58:18PM GMT, Eric Biggers wrote:
> Hi Christian,
> 
> On Wed, Jul 31, 2024 at 12:01:12PM +0200, Christian Brauner wrote:
> > It's currently possible to create pidfds for kthreads but it is unclear
> > what that is supposed to mean. Until we have use-cases for it and we
> > figured out what behavior we want block the creation of pidfds for
> > kthreads.
> > 
> > Fixes: 32fcb426ec00 ("pid: add pidfd_open()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  kernel/fork.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> Unfortunately this commit broke systemd-shutdown's ability to kill processes,
> which makes some filesystems no longer get unmounted at shutdown.
> 
> It looks like systemd-shutdown relies on being able to create a pidfd for any
> process listed in /proc (even a kthread), and if it gets EINVAL it treats it a
> fatal error and stops looking for more processes...

Thanks for the report!
I talked to Daan De Meyer who made that change and he said that this
must a systemd version that hasn't gotten his fixes yet. In any case, if
this causes regression then I'll revert it right now. See the appended
revert.

[-- Attachment #2: 0001-Revert-pidfd-prevent-creation-of-pidfds-for-kthreads.patch --]
[-- Type: text/x-diff, Size: 2038 bytes --]

From 780d60bac21ebee5c2465d21fe51b67bb9b054db Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Mon, 19 Aug 2024 10:38:23 +0200
Subject: [PATCH] Revert "pidfd: prevent creation of pidfds for kthreads"

This reverts commit 3b5bbe798b2451820e74243b738268f51901e7d0.

Eric reported that systemd-shutdown gets broken by blocking the creating
of pidfds for kthreads as older versions seems to rely on being able to
create a pidfd for any process in /proc.

Reported-by: Eric Biggers <ebiggers@kernel.org>
Link: https://lore.kernel.org/r/20240818035818.GA1929@sol.localdomain
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 kernel/fork.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 18bdc87209d0..cc760491f201 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2053,23 +2053,10 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
  */
 int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
 {
-	if (!pid)
-		return -EINVAL;
-
-	scoped_guard(rcu) {
-		struct task_struct *tsk;
-
-		if (flags & PIDFD_THREAD)
-			tsk = pid_task(pid, PIDTYPE_PID);
-		else
-			tsk = pid_task(pid, PIDTYPE_TGID);
-		if (!tsk)
-			return -EINVAL;
+	bool thread = flags & PIDFD_THREAD;
 
-		/* Don't create pidfds for kernel threads for now. */
-		if (tsk->flags & PF_KTHREAD)
-			return -EINVAL;
-	}
+	if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID))
+		return -EINVAL;
 
 	return __pidfd_prepare(pid, flags, ret);
 }
@@ -2416,12 +2403,6 @@ __latent_entropy struct task_struct *copy_process(
 	if (clone_flags & CLONE_PIDFD) {
 		int flags = (clone_flags & CLONE_THREAD) ? PIDFD_THREAD : 0;
 
-		/* Don't create pidfds for kernel threads for now. */
-		if (args->kthread) {
-			retval = -EINVAL;
-			goto bad_fork_free_pid;
-		}
-
 		/* Note that no task has been attached to @pid yet. */
 		retval = __pidfd_prepare(pid, flags, &pidfile);
 		if (retval < 0)
-- 
2.43.0