[LTP] [PATCH 3/3] Add process_mrelease02 test

[Andrea Cervesato]

Hi Cyril,

On 7/17/24 15:24, Cyril Hrubis wrote:
> On Wed, May 22, 2024 at 04:33:07PM +0200, Andrea Cervesato wrote:
>> From: Andrea Cervesato <andrea.cervesato@suse.com>
>>
>> This test verifies that process_mrelease() syscall correctly raises
>> the errors.
>>
>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
>> ---
>>   runtest/syscalls                                   |  1 +
>>   .../kernel/syscalls/process_mrelease/.gitignore    |  1 +
>>   .../syscalls/process_mrelease/process_mrelease02.c | 75 ++++++++++++++++++++++
>>   3 files changed, 77 insertions(+)
>>
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index 46a85fd31..c2fe919f0 100644
>> --- a/runtest/syscalls
>> +++ b/runtest/syscalls
>> @@ -1051,6 +1051,7 @@ preadv203_64 preadv203_64
>>   profil01 profil01
>>   
>>   process_mrelease01 process_mrelease01
>> +process_mrelease02 process_mrelease02
>>   
>>   process_vm_readv01 process_vm01 -r
>>   process_vm_readv02 process_vm_readv02
>> diff --git a/testcases/kernel/syscalls/process_mrelease/.gitignore b/testcases/kernel/syscalls/process_mrelease/.gitignore
>> index 673983858..f1e7a8fea 100644
>> --- a/testcases/kernel/syscalls/process_mrelease/.gitignore
>> +++ b/testcases/kernel/syscalls/process_mrelease/.gitignore
>> @@ -1 +1,2 @@
>>   /process_mrelease01
>> +/process_mrelease02
>> diff --git a/testcases/kernel/syscalls/process_mrelease/process_mrelease02.c b/testcases/kernel/syscalls/process_mrelease/process_mrelease02.c
>> new file mode 100644
>> index 000000000..ac13317ee
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/process_mrelease/process_mrelease02.c
>> @@ -0,0 +1,75 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * This test verifies that process_mrelease() syscall is raising errors:
>> + * * EBADF when a bad file descriptor is given
>> + * * EINVAL when flags is not zero
>> + * * EINVAL when memory of a task cannot be released because it's still running
>> + */
>> +
>> +#include "tst_test.h"
>> +#include "lapi/syscalls.h"
>> +
>> +static int badfd = -1;
>> +static int pidfd;
>> +
>> +static struct tcase {
>> +	int needs_child;
>> +	int *fd;
>> +	int flags;
>> +	int exp_errno;
>> +	char *msg;
>> +} tcases[] = {
>> +	{0, &badfd,  0, EBADF,  "bad file descriptor"},
>> +	{1, &pidfd, -1, EINVAL, "flags is not 0"},
>> +	{1, &pidfd,  0, EINVAL,  "memory of running task cannot be released"},
> We can easily trigger ESCHR as well, just fork a child that just exits,
> get pidfd to that child and then wait the child.
>
>> +};
>> +
>> +static void run(unsigned int n)
>> +{
>> +	struct tcase *tc = &tcases[n];
>> +
>> +	if (tc->needs_child) {
>> +		pid_t pid;
>> +
>> +		pid = SAFE_FORK();
>> +		if (!pid) {
>> +			tst_res(TINFO, "Keep child alive");
>> +
>> +			TST_CHECKPOINT_WAKE_AND_WAIT(0);
>> +			exit(0);
>> +		}
>> +
>> +		TST_CHECKPOINT_WAIT(0);
>> +
>> +		pidfd = SAFE_PIDFD_OPEN(pid, 0);
>> +	}
> We can set up several types of a child processes in the test setup,
> there is no need to do this on every iteration.
After working a bit on this I can say that LTP doesn't allow it. Simply 
because tst_reap_children() is called before the end of the test, which 
means we have to end all children, even if they need to be alive. And 
the have to be alive for the next step. So the first approach is the 
right one.
>
> Similarily for the ESCHR case we can just do the same for EINVAL cases.
> For the invalid flags we would need a process that did actually exit but
> wasn't waited for. And for the second EINVAL case we would need a
> running process, so perhaps just a child that sleeps in pause().
>
>> +	TST_EXP_FAIL(tst_syscall(__NR_process_mrelease, *tc->fd, tc->flags),
>> +		tc->exp_errno,
>> +		"%s", tc->msg);
>> +
>> +	if (tc->needs_child) {
>> +		SAFE_CLOSE(pidfd);
>> +
>> +		TST_CHECKPOINT_WAKE(0);
>> +	}
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test = run,
>> +	.tcnt = ARRAY_SIZE(tcases),
>> +	.needs_root = 1,
>> +	.forks_child = 1,
>> +	.min_kver = "5.15",
>> +	.needs_checkpoints = 1,
>> +	.needs_kconfigs = (const char *[]) {
>> +		"CONFIG_MMU=y",
> I do not think this is necessary, LTP does not run on non-MMU targets
> anyways.
>
>> +		NULL,
>> +	},
>> +};
> Also I think that it would make sense to CC Michal Hocko on the v2 since
> he did review the kernel patches for this syscall.
>
Andrea