Re: [LTP] [PATCH v3] getcpu: Add testcase for EFAULT

[Petr Vorel <pvorel@suse.cz>]

> Hi!

> On 8/5/24 11:22, Ma Xinjian via ltp wrote:
> > Add a testcase with the arguments point to an invalid address.

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> > Signed-off-by: Ma Xinjian <maxj.fnst@fujitsu.com>
> > ---
> >   runtest/syscalls                            |  1 +
> >   testcases/kernel/syscalls/getcpu/getcpu02.c | 97 +++++++++++++++++++++
> >   2 files changed, 98 insertions(+)
> >   create mode 100644 testcases/kernel/syscalls/getcpu/getcpu02.c

> > diff --git a/runtest/syscalls b/runtest/syscalls
> > index b8728c1c5..1537b5022 100644
> > --- a/runtest/syscalls
> > +++ b/runtest/syscalls
> > @@ -448,6 +448,7 @@ futimesat01 futimesat01
> >   getcontext01 getcontext01
> >   getcpu01 getcpu01
> > +getcpu02 getcpu02
> >   getcwd01 getcwd01
> >   getcwd02 getcwd02
> > diff --git a/testcases/kernel/syscalls/getcpu/getcpu02.c b/testcases/kernel/syscalls/getcpu/getcpu02.c
> > new file mode 100644
> > index 000000000..f32660ef9
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/getcpu/getcpu02.c
> > @@ -0,0 +1,97 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2024 FUJITSU LIMITED. All Rights Reserved.
> > + * Copyright (c) Linux Test Project, 2024
> > + * Author: Ma Xinjian <maxj.fnst@fujitsu.com>
> > + *
> > + */
> > +
> > +/*\
> > + * [Description]
> > + *
> > + * Verify that getcpu(2) fails with
> > + *
> > + * - EFAULT arguments point outside the calling process's address
> > + *          space.
> We can squeeze the description in one single line since EFAULT is the only
> one we are gonna test.

Good point. As Andrea suggested to use .tcnt, defining 2 testcases with
different getcpu() input values, these two would deserve to be described.

Also it'd be nice to add some tst_res(TINFO, ...) description in the testing
function (similar to one above in /*\.

Kind regards,
Petr

> > + */
> > +
> > +#define _GNU_SOURCE
> > +#include <errno.h>
> > +#include <sys/resource.h>
> > +#include <sys/time.h>
> > +#include <sys/wait.h>
> > +#include <stdlib.h>
> tst_test.h is importing these already.
> > +
> > +#include "tst_test.h"
> > +#include "lapi/sched.h"
> > +
> > +static void *bad_addr;
> > +
> > +static void setup(void)
> > +{
> > +	bad_addr = tst_get_bad_addr(NULL);
> > +}
> > +
> > +static void check_bad_cpu_id(void *bad_addr, unsigned int *node_id)
> > +{
> > +	int status;
> > +	pid_t pid;
> > +
> > +	pid = SAFE_FORK();
> > +	if (!pid) {
> > +		TST_EXP_FAIL(getcpu(bad_addr, node_id), EFAULT);
> > +
> > +		exit(0);
> > +	}
> > +
> > +	SAFE_WAITPID(pid, &status, 0);
> > +
> > +	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGSEGV) {
> > +		tst_res(TPASS, "getcpu() caused SIGSEGV");
> > +		return;
> > +	}
> > +
> > +	if (WIFEXITED(status) && WEXITSTATUS(status) == 0)
> > +		return;
> > +
> > +	tst_res(TFAIL, "child %s", tst_strstatus(status));
> > +}
> > +
> > +static void check_bad_node_id(unsigned int *cpu_id, void *bad_addr)
> > +{
> > +	int status;
> > +	pid_t pid;
> > +
> > +	pid = SAFE_FORK();
> > +	if (!pid) {
> > +		TST_EXP_FAIL(getcpu(cpu_id, bad_addr), EFAULT);
> > +
> > +		exit(0);
> > +	}
> > +
> > +	SAFE_WAITPID(pid, &status, 0);
> > +
> > +	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGSEGV) {
> > +		tst_res(TPASS, "getcpu() caused SIGSEGV");
> > +		return;
> > +	}
> > +
> > +	if (WIFEXITED(status) && WEXITSTATUS(status) == 0)
> > +		return;
> > +
> > +	tst_res(TFAIL, "child %s", tst_strstatus(status));
> > +}

These two testing functions are nearly the same, only parameters are different.
How about creating a single function and just pass a different params?
Also, only bad_addr and one of the functions holding "0" as id is needed.
> > +
> > +static void run_test(void)
> > +{
> > +	unsigned int cpu_id, node_id = 0;
> > +
> > +	check_bad_cpu_id(bad_addr, &node_id);
> > +	check_bad_node_id(&cpu_id, bad_addr);

> Here we can use .test/.tcnt , defining 2 testcases with different getcpu()
> input values.
+1

NOTE: sometimes we create struct tcase, but it'd be overkill for these 2
functions.

Kind regards,
Petr

> > +}
> > +
> > +static struct tst_test test = {
> > +	.setup = setup,
> > +	.test_all = run_test,
> > +	.forks_child = 1,
> > +};

> Best regards,
> Andrea Cervesato

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp