[PATCH v7 4/8] linux: Add posix_spawnattr_{get, set}cgroup_np (BZ 26731)

[Adhemerval Zanella Netto]

On 11/08/23 07:51, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> These function allow to posix_spawn and posix_spawnp to use
>> CLONE_INTO_CGROUP with clone3, allowing the child process to
>> be created in a different version 2 cgroup.  These are GNU
>> extensions that are available only for Linux, and also only
>> for the architectures that implement clone3 wrapper
>> (HAVE_CLONE3_WRAPPER).
>>
>> To create a process on a different cgroupv2, one can use the:
>>
>>   posix_spawnattr_t attr;
>>   posix_spawnattr_init (&attr);
>>   posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETCGROUP);
>>   posix_spawnattr_setcgroup_np (&attr, cgroup);
>>   posix_spawn (...)
> 
> Why are both POSIX_SPAWN_SETCGROUP and posix_spawnattr_setcgroup_np
> needed?  Couldn't the latter imply the former?

Besides being orthogonal with the other standard options, it allows
the called to just set/reset a flag in a posix_spawnattr_t to enable
disable the options instead of create/destroy a new attribute for
each posix_spawn call.

> 
>> There is no fallback is either clone3 does not support the flag
>> or if the architecture does not provide the clone3 wrapper, in
>> this case posix_spawn returns ENOTSUP.
> 
> I think this really should be added to the manual, mayb
> 
> It's also not clear to me how you would probe for support properly.
> The spawn operation might fail for other reasons.
> 
> I wonder if we have to probe as part of the 

Some comments seems to be truncated.  For probing, posix_spawnattr_setflags
fails with invalid flags, so trying to use POSIX_SPAWN_SETCGROUP if is
is not support should return EINVAL.

About the manual, I can add something but since we do not any sort of
posix_spawn it would require to add a lot of stubs.

> 
>> diff --git a/sysdeps/unix/sysv/linux/bits/spawn_ext.h b/sysdeps/unix/sysv/linux/bits/spawn_ext.h
>> new file mode 100644
>> index 0000000000..3bc10ab477
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/bits/spawn_ext.h
> 
>> +/* Get the cgroupsv2 the attribute structure.  */
>> +extern int posix_spawnattr_getcgroup_np (const posix_spawnattr_t *
>> +					 __restrict __attr,
>> +					 int *__cgroup)
>> +     __THROW __nonnull ((1, 2));
>> +
>> +/* Store scheduling parameters in the attribute structure.  */
>> +extern int posix_spawnattr_setcgroup_np (posix_spawnattr_t *__restrict __attr,
>> +					 int __cgroup)
>> +     __THROW __nonnull ((1));
> 
> Second comment seems wrong.

Indeed, and there is no need of __restrict here.  Also on 
posix_spawnattr_getcgroup_np it should have a __restrict for the
cgroup argument.

> 
>> diff --git a/sysdeps/unix/sysv/linux/tst-spawn-cgroup.c b/sysdeps/unix/sysv/linux/tst-spawn-cgroup.c
>> new file mode 100644
>> index 0000000000..6dba30ab29
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-spawn-cgroup.c
>> @@ -0,0 +1,216 @@
>> +/* Tests for posix_spawn cgroup extension.
> 
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
> 
> Should be “https://”.

Ack.

> 
>> +#define F_TYPE_EQUAL(a, b) (a == (typeof(a)) b)
> 
> Missing space after “type”.

Ack.

> 
>> +static char *
>> +get_cgroup (void)
>> +{
>> +  FILE *f = fopen ("/proc/self/cgroup", "re");
>> +  if (f == NULL)
>> +    FAIL_UNSUPPORTED ("no cgroup defined for the process");
> 
> Maybe add %m here.

Ack.

> 
>> +/* Called on process re-execution.  */
>> +_Noreturn static void
>> +handle_restart (int argc, char *argv[])
>> +{
>> +  assert (argc == 1);
>> +  char *newcgroup = argv[0];
>> +
>> +  char *current_cgroup = get_cgroup ();
>> +  TEST_VERIFY_EXIT (current_cgroup != NULL);
>> +  TEST_COMPARE_STRING (newcgroup, current_cgroup);
>> +  exit (EXIT_SUCCESS);
>> +}
> 
> I think the exit (EXIT_SUCCESS) masks failures because after execve, the
> shared mapping with failure status does not exist.

The TEST_VERIFY_EXIT should trigger the waitid checks, but you are right
for TEST_COMPARE_STRING. I removed the exit to let the support/test-driver.c
return the expected exit code.

> 
>> +static int
>> +create_new_cgroup (char **newcgroup)
>> +{
>> +  struct statfs fs;
>> +  if (statfs (CGROUPFS, &fs) < 0)
>> +    {
>> +      if (errno == ENOENT)
>> +	FAIL_UNSUPPORTED ("not cgroupv2 mount found");
>> +      FAIL_EXIT1 ("statfs (%s): %m\n", CGROUPFS);
> 
> “no[] cgroupv2 found?”

Ack.

> 
>> +    }
>> +
>> +  if (!F_TYPE_EQUAL (fs.f_type, CGROUP2_SUPER_MAGIC))
>> +    FAIL_UNSUPPORTED ("%s is not a cgroupv2", CGROUPFS);
> 
> This could print fs.f_type.

Ack.