On Mon, Aug 7, 2023 at 10:52 AM David Rheinsberg <[email protected]> wrote:
>
> A pidfd can currently only be created for tasks that are thread-group
> leaders and not reaped. This patch changes the pidfd-core to allow for
> pidfds on reapead thread-group leaders as well.
>
> A pidfd can outlive the task it refers to, and thus user-space must
> already be prepared that the task underlying a pidfd is gone at the time
> they get their hands on the pidfd. For instance, resolving the pidfd to
> a PID via the fdinfo must be prepared to read `-1`.
>
> Despite user-space knowing that a pidfd might be stale, several kernel
> APIs currently add another layer that checks for this. In particular,
> SO_PEERPIDFD returns `EINVAL` if the peer-task was already reaped,
> but returns a stale pidfd if the task is reaped immediately after the
> respective alive-check.
>
> This has the unfortunate effect that user-space now has two ways to
> check for the exact same scenario: A syscall might return
> EINVAL/ESRCH/... *or* the pidfd might be stale, even though there is no
> particular reason to distinguish both cases. This also propagates
> through user-space APIs, which pass on pidfds. They must be prepared to
> pass on `-1` *or* the pidfd, because there is no guaranteed way to get a
> stale pidfd from the kernel.
>
> This patch changes the core pidfd helpers to allow creation of pidfds
> even if the PID is no longer linked to any task. This only affects one
> of the three pidfd users that currently exist:
>
> 1) fanotify already tests for a linked TGID-task manually before
> creating the PIDFD, thus it is not directly affected by this change.
> However, note that the current fanotify code fails with an error if
> the target process is reaped exactly between the TGID-check in
> fanotify and the test in pidfd_prepare(). With this patch, this
> will no longer be the case.
>
> 2) pidfd_open(2) calls find_get_pid() before creating the pidfd, thus
> it is also not directly affected by this change.
> Again, similar to fanotify, there is a race between the
> find_get_pid() call and pidfd_prepare(), which currently causes
> pidfd_open(2) to return EINVAL rather than ESRCH if the process is
> reaped just between those two checks. With this patch, this will no
> longer be the case.
>
> 3) SO_PEERPIDFD will be affected by this change and from now on return
> stale pidfds rather than EINVAL if the respective peer task is
> reaped already.
>
> Given that users of SO_PEERPIDFD must already deal with stale pidfds,
> this change hopefully simplifies the API of SO_PEERPIDFD, and all
> dependent user-space APIs (e.g., GetConnectionCredentials() on D-Bus
> driver APIs). Also note that SO_PEERPIDFD is still pending to be
> released with linux-6.5.
>
> Signed-off-by: David Rheinsberg <[email protected]>
> ---
> kernel/fork.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index d2e12b6d2b18..4dde19a8c264 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2161,7 +2161,7 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
> * Allocate a new file that stashes @pid and reserve a new pidfd number in the
> * caller's file descriptor table. The pidfd is reserved but not installed yet.
> *
> - * The helper verifies that @pid is used as a thread group leader.
> + * The helper verifies that @pid is/was used as a thread group leader.
> *
> * If this function returns successfully the caller is responsible to either
> * call fd_install() passing the returned pidfd and pidfd file as arguments in
> @@ -2180,7 +2180,14 @@ 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 || !pid_has_task(pid, PIDTYPE_TGID))
> + if (!pid)
> + return -EINVAL;
> +
> + /*
> + * Non thread-group leaders cannot have pidfds, but we allow them for
> + * reaped thread-group leaders.
> + */
> + if (pid_has_task(pid, PIDTYPE_PID) && !pid_has_task(pid, PIDTYPE_TGID))
> return -EINVAL;
Hi David!
As far as I understand, __unhash_process is always called with a
tasklist_lock held for writing.
Don't we need to take tasklist_lock for reading here to guarantee
consistency between
pid_has_task(pid, PIDTYPE_PID) and pid_has_task(pid, PIDTYPE_TGID)
return values?
Kind regards,
Alex
>
> return __pidfd_prepare(pid, flags, ret);
> --
> 2.41.0
>
Hi
On Mon, Aug 7, 2023, at 11:01 AM, Alexander Mikhalitsyn wrote:
> On Mon, Aug 7, 2023 at 10:52 AM David Rheinsberg <[email protected]> wrote:
[...]
>> int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
>> {
>> - if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
>> + if (!pid)
>> + return -EINVAL;
>> +
>> + /*
>> + * Non thread-group leaders cannot have pidfds, but we allow them for
>> + * reaped thread-group leaders.
>> + */
>> + if (pid_has_task(pid, PIDTYPE_PID) && !pid_has_task(pid, PIDTYPE_TGID))
>> return -EINVAL;
>
> Hi David!
>
> As far as I understand, __unhash_process is always called with a
> tasklist_lock held for writing.
> Don't we need to take tasklist_lock for reading here to guarantee
> consistency between
> pid_has_task(pid, PIDTYPE_PID) and pid_has_task(pid, PIDTYPE_TGID)
> return values?
You mean PIDTYPE_TGID being cleared before PIDTYPE_PID (at least from the perspective of the unlocked reader)? I don't think it is a compatibility issue, because the same issue existed before the patch. But it might indeed be required to avoid spurious EINVAL _while_ the target process is reaped.
It would be unfortunate if we need that. Because it is really not required for AF_UNIX or fanotify (they guarantee that they always deal with TGIDs). So maybe the correct call is to just drop pidfd_prepare() and always use __pidfd_prepare()? So far the safety-measures of pidfd_prepare() introduced two races I already mentioned in the commit-message. So maybe it is just better to document that the caller of __pidfd_prepare() needs to ensure the source is/was a TGID?
Thanks
David