Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 1 Apr 2022 11:42:25 +0200
From:   Christian Brauner <brauner@kernel.org>
To:     Alois Wohlschlager <alwoju@gmx.de>
Cc:     "Eric W. Biederman" <ebiederm@xmission.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Alexey Gladkov <legion@kernel.org>,
        Jens Axboe <axboe@kernel.dk>,
        David Hildenbrand <david@redhat.com>,
        Rolf Eike Beer <eb@emlix.com>,
        Ran Xiaokai <ran.xiaokai@zte.com.cn>,
        Matthew Bobrowski <repnop@google.com>, Jan Kara <jack@suse.cz>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] pid: Allow creation of pidfds to threads
Message-ID: <20220401094225.epianrz3afrhfrqp@wittgenstein>
References: <3412128.IC5jYiYEAv@genesis>
 <20220401070942.odjbuc5wecfayyok@wittgenstein>
 <5392366.5i5WIIk9Ns@genesis>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <5392366.5i5WIIk9Ns@genesis>
Precedence: bulk

On Fri, Apr 01, 2022 at 11:00:27AM +0200, Alois Wohlschlager wrote:
> Hello Christian,
> 
> > We originally blocked this because it is not as easy as simply allowing
> > pidfds to be created for non-thread-group leaders.
> > For a start, pidfd_poll() currently doens't work if pidfd_task() isn't a
> > thread-group leader
> 
> I did notice the hang there, that's why my patch changes pidfd_poll to return
> error on tasks which are not thread-group leaders. IIRC, waiting on specific
> threads is not supported by Linux at all, so I don't see a problem with not
> supporting it here either.

In general, it would be quite neat if we could get notified about thread
exit through poll though. That'd be pretty useful. But maybe it's indeed
ok to just not support this (for now at least).

I know that systemd is using pidfds in their event loop so I'd need to
see whether they'd want support for this behavior.

> 
> > and you'll just hang for CLONE_PIDFD | CLONE_THREAD.
> 
> No, CLONE_PIDFD | CLONE_THREAD behavior is unchanged, it will still fail with
> EINVAL. I actually confirmed this by double-checking right now.

I just used the two flags as a shorthand for pidfds referring to
threads. That might've been misleading here.

> 
> > So at least that needs to be adapated as well and there's likely a bunch
> > of other corner-cases I'm forgetting about.
> 
> I'd be happy to hear about other corner-cases so I can fix them.

I need to play with this patch a little and see what current
expectations we do have in the code.

There are various consumers of pidfds and they all have been added with
the assumption that a pidfd refers to a thread-group leader. We should
go through them and see whether changing them to operate on threads is
sane before we can just switch the generic helper.

Bot process_madvise() and process_mrelease() should be fine to operate
on threads afaict from the discussion when they were added.

For pidfd_send_signal() we likely want to at least consider adding the
ability to send a thread-specific signal, i.e. supporting tgkill()
behavior. As it stands it currently only supports kill()-like behavior
where the signal that gets sent is thread-group directed.

I roughly had originally envisioned this to be supportable through the
addition of a new flag to pidfd_send_signal() so callers would be able
to select whether to send a thread-specific signal or not. What do
people think of that?

> 
> > Do you have a concrete use-case you want this for?
> 
> My use-case is basically making pidfd_getfd actually useful for its intended
> purpose: there is a seccomp_unotify-based supervisor that wants to obtain a
> file descriptor from its guest. This currently does not work if the action to
> be forwarded to the supervisor is performed in a secondary thread, since there
> is no way to obtain the required pidfd.

Yeah, I'm well aware of that. I've been working around this limitation
in our implementation for the seccomp notifier for quite a long time
when intercepting the bpf()-syscall.