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

[Adhemerval Zanella Netto]

On 24/08/23 04:13, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Returning a pidfd allows a process to keep a race-free handle for a
>> child process, otherwise, the caller will need to either use pidfd_open
>> (which still might be subject to TOCTOU) or keep the old racy interface
>> base on pid_t.
>>
>> The implementation makes sure that kernel must support the complete
>> pidfd interface, meaning that waitid (P_PIDFD) should be supported
>> (added on Linux 5.4).  It ensures that a non-racy workaround is required
>> (such as reading procfs fdinfo pid to use along with wait interfaces).
> 
> Sorry, I don't understand the second sentence.

It is indeed confusing, I will change to:

To correctly use pifd_spawn, the kernel must support not only returning 
the pidfd with clone/clone3 but also waitid (P_PIDFD) (added on Linux 5.4).
If the kernel does not support the waitid, pidfd returns ENOSYS.  It avoids 
the need for racy workarounds, such as reading the procfs fdinfo to get the 
pid to use along with other wait interfaces.

> 
>> diff --git a/posix/tst-spawn3.c b/posix/tst-spawn3.c
>> index e7ce0fb386..64052dc911 100644
>> --- a/posix/tst-spawn3.c
>> +++ b/posix/tst-spawn3.c
>> @@ -16,6 +16,7 @@
>>     License along with the GNU C Library; if not, see
>>     <https://www.gnu.org/licenses/>.  */
>>  
>> +#include <assert.h>
> 
> Please use TEST_VERIFY_EXIT, see below.
> 
>> @@ -75,75 +78,82 @@ do_test (void)
>>  	    FAIL_EXIT1 ("create_temp_file: %m");
>>  	  break;
>>  	}
>> -      files[nfiles++] = fd;
>> +      files[nfiles] = fd;
>>      }
>> +  assert (nfiles != 0);
> 
> TEST_VERIFY_EXIT (nfiles != 0);

Ack.

> 
>> diff --git a/sysdeps/unix/sysv/linux/bits/spawn_ext.h b/sysdeps/unix/sysv/linux/bits/spawn_ext.h
>> index a3aa020d5c..3254cfe9be 100644
>> --- a/sysdeps/unix/sysv/linux/bits/spawn_ext.h
>> +++ b/sysdeps/unix/sysv/linux/bits/spawn_ext.h
>> @@ -37,4 +37,35 @@ extern int posix_spawnattr_setcgroup_np (posix_spawnattr_t *__attr,
>>  
>>  #endif /* __USE_MISC */
>>  
>> +#ifdef __USE_GNU
> 
> Please use __USE_MISC, so this is available with _DEFAULT_SOURCE (like
> the cgroups functions).

Ack.

> 
>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>> index f0d4c62ae6..d4ff23d955 100644
>> --- a/sysdeps/unix/sysv/linux/spawni.c
>> +++ b/sysdeps/unix/sysv/linux/spawni.c
> 
>>    internal_signal_block_all (&args.oldmask);
>> @@ -386,13 +399,16 @@ __spawnix (pid_t * pid, const char *file,
>>        /* Unsupported flags like CLONE_CLEAR_SIGHAND will be cleared up by
>>  	 __clone_internal_fallback.  */
>>        .flags = (set_cgroup ? CLONE_INTO_CGROUP : 0)
>> +	       | (use_pidfd ? CLONE_PIDFD : 0)
>>  	       | CLONE_CLEAR_SIGHAND
>>  	       | CLONE_VM
>>  	       | CLONE_VFORK,
>>        .exit_signal = SIGCHLD,
>>        .stack = (uintptr_t) stack,
>>        .stack_size = stack_size,
>> -      .cgroup = (set_cgroup ? attrp->__cgroup : 0)
>> +      .cgroup = (set_cgroup ? attrp->__cgroup : 0),
>> +      .pidfd = use_pidfd ? (uintptr_t) &args.pidfd : 0,
>> +      .parent_tid = use_pidfd ? (uintptr_t) &args.pidfd : 0,
> 
> The .parent_tid line looks wrong?

It is required for clone (and that's why you can't use CLONE_PIDFD with
CLONE_PARENT_SETTID). It could only set parent_tid on clone fallback,
but I think this is simpler.  I will add a comment.