[LTP] [PATCH v4] getcpu: Add testcase for EFAULT
Xinjian Ma (Fujitsu)
maxj.fnst@fujitsu.com
Fri Aug 9 09:00:06 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
Hi Petr, Andrea
Thank you for your patient review, I have submitted the [PATCH v5], PTAL.
Best regards
Ma
More information about the ltp
mailing list