[LTP] [PATCH v4] getcpu: Add testcase for EFAULT
Petr Vorel
pvorel@suse.cz
Thu Aug 8 11:16:57 CEST 2024
Hi Ma,
> Add a testcase with the arguments point to an invalid address.
Generally LGTM, I have few comments, but I'll fix them before merge.
> Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.de>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Ma Xinjian <maxj.fnst@fujitsu.com>
@Andrea, Cyril, feel free to have final look during today (otherwise I'll merge
later today).
@Ma The test has changed 3 times quite significantly. IMHO it's better not to
add Reviewed-by: unless you change just either simple thing or did changes you
were explicitly asked to do. Better is to --cc all people who did the review
when generating patches with 'git format-patch'.
(It's good to add Reviewed-by: if your patchset has more commits and some of
them were previously reviewed and they are unchanged in the following version.)
> ---
Also, not needed, but it helps, if you wrote some changes to the previous
version after these '---' (it will not be part of the git commit message when
other developers apply your patch with 'git am'.
> runtest/syscalls | 1 +
> testcases/kernel/syscalls/getcpu/getcpu02.c | 71 +++++++++++++++++++++
> 2 files changed, 72 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
You miss adding "/getcpu02" into testcases/kernel/syscalls/getcpu/.gitignore.
I'll do that before merge.
> 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..859cb0d3e
> --- /dev/null
> +++ b/testcases/kernel/syscalls/getcpu/getcpu02.c
> @@ -0,0 +1,71 @@
> +// 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:
> + *
> + * 1) cpu_id points outside the calling process's address space.
> + * 2) node_id points outside the calling process's address space.
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include "tst_test.h"
> +#include "lapi/sched.h"
> +
> +static unsigned int cpu_id, node_id;
> +
> +static struct tcase {
> + unsigned int *cpu_id;
> + unsigned int *node_id;
> +} tcases[] = {
> + {NULL, &node_id},
> + {&cpu_id, NULL}
I meant to add char *desc here instead of tst_res(TINFO later in check_getcpu()
> +};
> +
> +static void check_getcpu(unsigned int n)
> +{
> + struct tcase *tc = &tcases[n];
> + int status;
> + pid_t pid;
> +
> + if (n == 0) {
It might be better to check this as:
if (!tc->cpu_id)
(set cpu_id only if not set.
> + tst_res(TINFO, "Make cpu_id point outside the calling process's address space.");
> + tc->cpu_id = tst_get_bad_addr(NULL);
> + } else if (n == 1) {
and here either if (!tc->node_id) or simple else.
> + tst_res(TINFO, "Make node_id point outside the calling process's address space.");
> + tc->node_id = tst_get_bad_addr(NULL);
Also, we usually try to set values which does not change in the setup function
(because one can run test more times, e.g. 1000x with :/getcpu02 -i1000. Thus
things which don't have to be repeated go to the setup function.
But when I tried that, even without setup function, using this pointer always
causes SIGSEGV (test passes). And I use direct static values it does *not* cause
SIGSEGV (test fails):
static unsigned int cpu_id, node_id;
static unsigned int *p_cpu_id = &cpu_id, *p_node_id = &node_id;
static struct tcase {
unsigned int **cpu_id;
unsigned int **node_id;
char *desc;
} tcases[] = {
{NULL, NULL, "none"},
{NULL, &p_node_id, "cpu_id"},
{&p_cpu_id, NULL, "node_id"},
};
static void check_getcpu(unsigned int n)
{
struct tcase *tc = &tcases[n];
int status;
pid_t pid;
tst_res(TINFO, "Test %s outside process's address space", tc->desc);
pid = SAFE_FORK();
if (!pid) {
TST_EXP_FAIL(getcpu(p_cpu_id, p_node_id), EFAULT); // this would always pass
TST_EXP_FAIL(getcpu(*tc->cpu_id, *tc->node_id), EFAULT); // this always fail, even for none
exit(0);
}
I guess I miss something obvious. Therefore I suggest to merge (or see the diff
below).
Kind regards,
Petr
static struct tcase {
unsigned int *cpu_id;
unsigned int *node_id;
char *desc;
} tcases[] = {
{NULL, &node_id, "cpu_id"},
{&cpu_id, NULL, "node_id"},
};
static void check_getcpu(unsigned int n)
{
struct tcase *tc = &tcases[n];
int status;
pid_t pid;
tst_res(TINFO, "Test %s outside process's address space", tc->desc);
if (!tc->cpu_id)
tc->cpu_id = tst_get_bad_addr(NULL);
if (!tc->node_id)
tc->node_id = tst_get_bad_addr(NULL);
pid = SAFE_FORK();
if (!pid) {
TST_EXP_FAIL(getcpu(tc->cpu_id, tc->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));
}
diff --git testcases/kernel/syscalls/getcpu/.gitignore testcases/kernel/syscalls/getcpu/.gitignore
index 31fec5d35e..cd3022bbb1 100644
--- testcases/kernel/syscalls/getcpu/.gitignore
+++ testcases/kernel/syscalls/getcpu/.gitignore
@@ -1 +1,2 @@
/getcpu01
+/getcpu02
diff --git testcases/kernel/syscalls/getcpu/getcpu02.c testcases/kernel/syscalls/getcpu/getcpu02.c
index 859cb0d3eb..83d236a78c 100644
--- testcases/kernel/syscalls/getcpu/getcpu02.c
+++ testcases/kernel/syscalls/getcpu/getcpu02.c
@@ -9,7 +9,7 @@
/*\
* [Description]
*
- * Verify that getcpu(2) fails with EFAULT:
+ * Verify that getcpu(2) fails with EFAULT if:
*
* 1) cpu_id points outside the calling process's address space.
* 2) node_id points outside the calling process's address space.
@@ -25,9 +25,10 @@ static unsigned int cpu_id, node_id;
static struct tcase {
unsigned int *cpu_id;
unsigned int *node_id;
+ char *desc;
} tcases[] = {
- {NULL, &node_id},
- {&cpu_id, NULL}
+ {NULL, &node_id, "cpu_id"},
+ {&cpu_id, NULL, "node_id"},
};
static void check_getcpu(unsigned int n)
@@ -36,13 +37,13 @@ static void check_getcpu(unsigned int n)
int status;
pid_t pid;
- if (n == 0) {
- tst_res(TINFO, "Make cpu_id point outside the calling process's address space.");
+ tst_res(TINFO, "Test %s outside process's address space", tc->desc);
+
+ if (!tc->cpu_id)
tc->cpu_id = tst_get_bad_addr(NULL);
- } else if (n == 1) {
- tst_res(TINFO, "Make node_id point outside the calling process's address space.");
+
+ if (!tc->node_id)
tc->node_id = tst_get_bad_addr(NULL);
- }
pid = SAFE_FORK();
if (!pid) {
More information about the ltp
mailing list