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

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240801-report-strukturiert-48470c1ac4e8@brauner>
Date: Thu, 1 Aug 2024 08:58:05 +0200
From: Christian Brauner <brauner@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Aleksa Sarai <cyphar@...har.com>, Tycho Andersen <tandersen@...flix.com>, 
	Daan De Meyer <daan.j.demeyer@...il.com>, Tejun Heo <tj@...nel.org>, stable@...r.kernel.org
Subject: Re: [PATCH] pidfd: prevent creation of pidfds for kthreads

On Wed, Jul 31, 2024 at 04:51:33PM GMT, Oleg Nesterov wrote:
> On 07/31, 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.
> 
> Hmm... could you explain your concerns? Why do you think we should disallow
> pidfd_open(pid-of-kthread) ?

It basically just works now and it's not intentional - at least not on
my part. You can't send signals to them, you may or may not get notified
via poll when a kthread exits. If we ever want this to be useful I would
like to enable it explicitly.

Plus, this causes confusion in userspace. When you have qemu running
with kvm support then kvm creates several kthreads (that inherit the
cgroup of the calling process). If you try to kill those instances via
systemctl kill or systemctl stop then pidfds for these kthreads are
opened but sending a signal to them is meaningless.

(So imho this causes more confusion then it is actually helpful. If we
add supports for kthreads I'd also like pidfs to gain a way to identify
them via statx() or fdinfo.)

> > @@ -2403,6 +2416,12 @@ __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;
> 
> Do we really need this check? Userspace can't use args->kthread != NULL,
> the kernel users should not use CLONE_PIDFD.

Yeah, I know. That's really just proactive so that user of e.g.,
copy_process() such as vhost or so on don't start handing out pidfds for
stuff without requring changes to the helper itself.