Re: [PATCH v8 5/7] posix: Add pidfd_spawn and pidfd_spawnp (BZ 30349)

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 24/08/23 15:18, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 24/08/23 14:00, Florian Weimer wrote:
>>> Please use a separate variable, not args.pidfd, though.  The current
>>> code depends on the order the kernel sets these fields, I think.
>>
>> I can move out the pidfd out of posix_spawn_args, but I don't think this
>> would change much here.  It would be a stack allocated variable in both
>> cases, and kernel will just set it if CLONE_PIDFD (afaik there is no
>> order involved here).
> 
> I'm concerned about this:
> 
>>>>>> +      .pidfd = use_pidfd ? (uintptr_t) &args.pidfd : 0,
>>>>>> +      .parent_tid = use_pidfd ? (uintptr_t) &args.pidfd : 0,
> 
> &args.pidfd in both cases.

My understanding is since it does use CLONE_PARENT_SETTID, if clone3 is
called it will ignore parent_tid and only set pidfd.  If clone is called,
there is no pidfd argument, and the pidfd will be returned on parent_tid
argument.  The kernel explicit disable CLONE_PIDFD | CLONE_PARENT_SETTID:

kernel/fork.c

2880         /*
2881          * For legacy clone() calls, CLONE_PIDFD uses the parent_tid argument
2882          * to return the pidfd. Hence, CLONE_PIDFD and CLONE_PARENT_SETTID are
2883          * mutually exclusive. With clone3() CLONE_PIDFD has grown a separate
2884          * field in struct clone_args and it still doesn't make sense to have
2885          * them both point at the same memory location. Performing this check
2886          * here has the advantage that we don't need to have a separate helper
2887          * to check for legacy clone().
2888          */
2889         if ((args->flags & CLONE_PIDFD) &&
2890             (args->flags & CLONE_PARENT_SETTID) &&
2891             (args->pidfd == args->parent_tid))
2892                 return -EINVAL;