[LTP] [PATCH v4] getcpu: Add testcase for EFAULT
Andrea Cervesato
andrea.cervesato@suse.com
Thu Aug 8 11:25:42 CEST 2024
Hi Ma,
On 8/8/24 11:16, Petr Vorel wrote:
> 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'.
Yes, please add Reviewed-by tag only when the reviewer replied with the
tag. That actually means "patch is ok for me, so it can be submitted"
instead of "i took a look at the patch and it needs to be modified".
>
> (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) {
Regards,
Andrea Cervesato
More information about the ltp
mailing list