| Message ID | 20241203-madvise_guard_install-v1-2-c0485abbfc73@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Coverage for MADV_GUARD_* features | expand |
Hi Andrea, [ I dared to Cc Lorenzo, the author of the kernel implementation ] > Verify that MADV_GUARD_INSTALL is causing SIGSEGV when someone is > trying to access memory advised with it. Test LGTM, thanks for covering a new kernel feature. Closes: https://github.com/linux-test-project/ltp/issues/1210 > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > --- > runtest/syscalls | 1 + > testcases/kernel/syscalls/madvise/.gitignore | 1 + > testcases/kernel/syscalls/madvise/madvise12.c | 102 ++++++++++++++++++++++++++ > 3 files changed, 104 insertions(+) > diff --git a/runtest/syscalls b/runtest/syscalls > index 5fd62617df1a116b1d94c57ff30f74693320a2ab..ded035ee82d0e97c67cc1e7c487b010634b2d1a0 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -1000,6 +1000,7 @@ madvise08 madvise08 > madvise09 madvise09 > madvise10 madvise10 > madvise11 madvise11 > +madvise12 madvise12 > newuname01 newuname01 > diff --git a/testcases/kernel/syscalls/madvise/.gitignore b/testcases/kernel/syscalls/madvise/.gitignore > index 722ac3c34306bac414313f1ce36ca98d715cd04c..758e601a9c4e7682a925f16184d14f2357009bc2 100644 > --- a/testcases/kernel/syscalls/madvise/.gitignore > +++ b/testcases/kernel/syscalls/madvise/.gitignore > @@ -8,3 +8,4 @@ > /madvise09 > /madvise10 > /madvise11 > +/madvise12 > diff --git a/testcases/kernel/syscalls/madvise/madvise12.c b/testcases/kernel/syscalls/madvise/madvise12.c > new file mode 100644 > index 0000000000000000000000000000000000000000..2bdf843f016a7c9d175a31b76ae805d63c4cbc80 > --- /dev/null > +++ b/testcases/kernel/syscalls/madvise/madvise12.c > @@ -0,0 +1,102 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2024 Andrea Cervesato <andrea.cervesato@suse.com> > + */ > + > +/*\ > + * [Description] > + * > + * Verify that MADV_GUARD_INSTALL is causing SIGSEGV when someone is accessing > + * memory advised with it. Maybe add: * This is a test for feature implemented in * 662df3e5c376 ("mm: madvise: implement lightweight guard page mechanism") The rest LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr > + * > + * [Algorithm] > + * > + * - allocate a certain amount of memory > + * - advise memory with MADV_GUARD_INSTALL > + * - access to memory from within a child and verify it gets killed by SIGSEGV > + * - release memory with MADV_GUARD_REMOVE > + * - verify that memory has not been modified before child got killed > + * - modify memory within a new child > + * - verify that memory is accessable and child was not killed by SIGSEGV > + */ > + > +#include "tst_test.h" > +#include "lapi/mmap.h" > + > +#define MAP_SIZE (8 * TST_KB) > + > +static char *addr; > + > +static void run(void) > +{ > + pid_t pid; > + int status; > + > + memset(addr, 0, MAP_SIZE); > + > + TST_EXP_PASS(madvise(addr, MAP_SIZE, MADV_GUARD_INSTALL)); > + > + pid = SAFE_FORK(); > + if (!pid) { > + tst_res(TINFO, "Modifying memory content"); > + > + memset(addr, 'a', MAP_SIZE); > + exit(0); > + } > + > + SAFE_WAITPID(pid, &status, 0); > + > + if (WIFSIGNALED(status) && WTERMSIG(status) == SIGSEGV) > + tst_res(TPASS, "Child ended by SIGSEGV as expected"); > + else > + tst_res(TFAIL, "Child: %s", tst_strstatus(status)); > + > + TST_EXP_PASS(madvise(addr, MAP_SIZE, MADV_GUARD_REMOVE)); > + > + for (int i = 0; i < MAP_SIZE; i++) { > + if (addr[i] == 'a') { > + tst_res(TFAIL, "Memory content has been modified"); > + return; > + } > + } > + > + tst_res(TPASS, "Memory content didn't change"); > + > + pid = SAFE_FORK(); > + if (!pid) { > + tst_res(TINFO, "Modifying memory content"); > + > + memset(addr, 'b', MAP_SIZE); > + exit(0); > + } > + > + SAFE_WAITPID(pid, &status, 0); > + > + if (!WIFSIGNALED(status)) > + tst_res(TPASS, "Child ended without being signaled"); > + else > + tst_res(TFAIL, "Child ended with %s", tst_strstatus(status)); > +} > + > +static void setup(void) > +{ > + addr = SAFE_MMAP(NULL, MAP_SIZE, > + PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, > + -1, 0); > +} > + > +static void cleanup(void) > +{ > + if (addr) > + SAFE_MUNMAP(addr, MAP_SIZE); > +} > + > +static struct tst_test test = { > + .test_all = run, > + .setup = setup, > + .cleanup = cleanup, > + .needs_root = 1, > + .forks_child = 1, > + .min_kver = "6.13", > +};
On Wed, Dec 11, 2024 at 01:30:48AM +0100, Petr Vorel wrote: > Hi Andrea, > > [ I dared to Cc Lorenzo, the author of the kernel implementation ] I'm not that scary am I? :))) Feel free to cc- me on anything relating to this even if obviously in the LTP project happy to be included! :) > > > Verify that MADV_GUARD_INSTALL is causing SIGSEGV when someone is > > trying to access memory advised with it. > > Test LGTM, thanks for covering a new kernel feature. All good, looks fine to me, though might be worth expanding over time, we have some self tests in the kernel for this, see tools/testing/selftests/mm/guard-pages.c. But it's nice to have a basic LTP regression test to assert the fundamental thing is working as it should, and also nice that you implement it from your perspective rather than mine, where I am obviously rather influenced by implementation details. Also note I submitted man pages for the change, you can pull it from [0] and view it via: $ man --manpath=. 2 madvise [0]:git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git Hopefully these will get distributed around soon! Thanks for doing this for my feature, much appreciated overall! Cheers, Lorenzo > > Closes: https://github.com/linux-test-project/ltp/issues/1210 > > > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > > --- > > runtest/syscalls | 1 + > > testcases/kernel/syscalls/madvise/.gitignore | 1 + > > testcases/kernel/syscalls/madvise/madvise12.c | 102 ++++++++++++++++++++++++++ > > 3 files changed, 104 insertions(+) > > > diff --git a/runtest/syscalls b/runtest/syscalls > > index 5fd62617df1a116b1d94c57ff30f74693320a2ab..ded035ee82d0e97c67cc1e7c487b010634b2d1a0 100644 > > --- a/runtest/syscalls > > +++ b/runtest/syscalls > > @@ -1000,6 +1000,7 @@ madvise08 madvise08 > > madvise09 madvise09 > > madvise10 madvise10 > > madvise11 madvise11 > > +madvise12 madvise12 > > > newuname01 newuname01 > > > diff --git a/testcases/kernel/syscalls/madvise/.gitignore b/testcases/kernel/syscalls/madvise/.gitignore > > index 722ac3c34306bac414313f1ce36ca98d715cd04c..758e601a9c4e7682a925f16184d14f2357009bc2 100644 > > --- a/testcases/kernel/syscalls/madvise/.gitignore > > +++ b/testcases/kernel/syscalls/madvise/.gitignore > > @@ -8,3 +8,4 @@ > > /madvise09 > > /madvise10 > > /madvise11 > > +/madvise12 > > diff --git a/testcases/kernel/syscalls/madvise/madvise12.c b/testcases/kernel/syscalls/madvise/madvise12.c > > new file mode 100644 > > index 0000000000000000000000000000000000000000..2bdf843f016a7c9d175a31b76ae805d63c4cbc80 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/madvise/madvise12.c > > @@ -0,0 +1,102 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2024 Andrea Cervesato <andrea.cervesato@suse.com> > > + */ > > + > > +/*\ > > + * [Description] > > + * > > + * Verify that MADV_GUARD_INSTALL is causing SIGSEGV when someone is accessing > > + * memory advised with it. > > Maybe add: > * This is a test for feature implemented in > * 662df3e5c376 ("mm: madvise: implement lightweight guard page mechanism") > > The rest LGTM. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > Petr > > > + * > > + * [Algorithm] > > + * > > + * - allocate a certain amount of memory > > + * - advise memory with MADV_GUARD_INSTALL > > + * - access to memory from within a child and verify it gets killed by SIGSEGV > > + * - release memory with MADV_GUARD_REMOVE > > + * - verify that memory has not been modified before child got killed > > + * - modify memory within a new child > > + * - verify that memory is accessable and child was not killed by SIGSEGV > > + */ > > + > > +#include "tst_test.h" > > +#include "lapi/mmap.h" > > + > > +#define MAP_SIZE (8 * TST_KB) > > + > > +static char *addr; > > + > > +static void run(void) > > +{ > > + pid_t pid; > > + int status; > > + > > + memset(addr, 0, MAP_SIZE); > > + > > + TST_EXP_PASS(madvise(addr, MAP_SIZE, MADV_GUARD_INSTALL)); > > + > > + pid = SAFE_FORK(); > > + if (!pid) { > > + tst_res(TINFO, "Modifying memory content"); > > + > > + memset(addr, 'a', MAP_SIZE); > > + exit(0); > > + } > > + > > + SAFE_WAITPID(pid, &status, 0); > > + > > + if (WIFSIGNALED(status) && WTERMSIG(status) == SIGSEGV) > > + tst_res(TPASS, "Child ended by SIGSEGV as expected"); > > + else > > + tst_res(TFAIL, "Child: %s", tst_strstatus(status)); > > + > > + TST_EXP_PASS(madvise(addr, MAP_SIZE, MADV_GUARD_REMOVE)); > > + > > + for (int i = 0; i < MAP_SIZE; i++) { > > + if (addr[i] == 'a') { > > + tst_res(TFAIL, "Memory content has been modified"); > > + return; > > + } > > + } > > + > > + tst_res(TPASS, "Memory content didn't change"); > > + > > + pid = SAFE_FORK(); > > + if (!pid) { > > + tst_res(TINFO, "Modifying memory content"); > > + > > + memset(addr, 'b', MAP_SIZE); > > + exit(0); > > + } > > + > > + SAFE_WAITPID(pid, &status, 0); > > + > > + if (!WIFSIGNALED(status)) > > + tst_res(TPASS, "Child ended without being signaled"); > > + else > > + tst_res(TFAIL, "Child ended with %s", tst_strstatus(status)); > > +} > > + > > +static void setup(void) > > +{ > > + addr = SAFE_MMAP(NULL, MAP_SIZE, > > + PROT_READ | PROT_WRITE, > > + MAP_PRIVATE | MAP_ANONYMOUS, > > + -1, 0); > > +} > > + > > +static void cleanup(void) > > +{ > > + if (addr) > > + SAFE_MUNMAP(addr, MAP_SIZE); > > +} > > + > > +static struct tst_test test = { > > + .test_all = run, > > + .setup = setup, > > + .cleanup = cleanup, > > + .needs_root = 1, > > + .forks_child = 1, > > + .min_kver = "6.13", > > +}; > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Thanks for the review. Pushed with suggested changes. Andrea On 12/11/24 10:29, Lorenzo Stoakes via ltp wrote: > On Wed, Dec 11, 2024 at 01:30:48AM +0100, Petr Vorel wrote: >> Hi Andrea, >> >> [ I dared to Cc Lorenzo, the author of the kernel implementation ] > I'm not that scary am I? :))) > > Feel free to cc- me on anything relating to this even if obviously in the > LTP project happy to be included! :) > >>> Verify that MADV_GUARD_INSTALL is causing SIGSEGV when someone is >>> trying to access memory advised with it. >> Test LGTM, thanks for covering a new kernel feature. > All good, looks fine to me, though might be worth expanding over time, we > have some self tests in the kernel for this, see > tools/testing/selftests/mm/guard-pages.c. > > But it's nice to have a basic LTP regression test to assert the fundamental > thing is working as it should, and also nice that you implement it from > your perspective rather than mine, where I am obviously rather influenced > by implementation details. > > Also note I submitted man pages for the change, you can pull it from [0] > and view it via: > > $ man --manpath=. 2 madvise > > [0]:git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git > > Hopefully these will get distributed around soon! > > Thanks for doing this for my feature, much appreciated overall! > > Cheers, Lorenzo > >> Closes: https://github.com/linux-test-project/ltp/issues/1210 >> >>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> >>> --- >>> runtest/syscalls | 1 + >>> testcases/kernel/syscalls/madvise/.gitignore | 1 + >>> testcases/kernel/syscalls/madvise/madvise12.c | 102 ++++++++++++++++++++++++++ >>> 3 files changed, 104 insertions(+) >>> diff --git a/runtest/syscalls b/runtest/syscalls >>> index 5fd62617df1a116b1d94c57ff30f74693320a2ab..ded035ee82d0e97c67cc1e7c487b010634b2d1a0 100644 >>> --- a/runtest/syscalls >>> +++ b/runtest/syscalls >>> @@ -1000,6 +1000,7 @@ madvise08 madvise08 >>> madvise09 madvise09 >>> madvise10 madvise10 >>> madvise11 madvise11 >>> +madvise12 madvise12 >>> newuname01 newuname01 >>> diff --git a/testcases/kernel/syscalls/madvise/.gitignore b/testcases/kernel/syscalls/madvise/.gitignore >>> index 722ac3c34306bac414313f1ce36ca98d715cd04c..758e601a9c4e7682a925f16184d14f2357009bc2 100644 >>> --- a/testcases/kernel/syscalls/madvise/.gitignore >>> +++ b/testcases/kernel/syscalls/madvise/.gitignore >>> @@ -8,3 +8,4 @@ >>> /madvise09 >>> /madvise10 >>> /madvise11 >>> +/madvise12 >>> diff --git a/testcases/kernel/syscalls/madvise/madvise12.c b/testcases/kernel/syscalls/madvise/madvise12.c >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..2bdf843f016a7c9d175a31b76ae805d63c4cbc80 >>> --- /dev/null >>> +++ b/testcases/kernel/syscalls/madvise/madvise12.c >>> @@ -0,0 +1,102 @@ >>> +// SPDX-License-Identifier: GPL-2.0-or-later >>> +/* >>> + * Copyright (c) 2024 Andrea Cervesato <andrea.cervesato@suse.com> >>> + */ >>> + >>> +/*\ >>> + * [Description] >>> + * >>> + * Verify that MADV_GUARD_INSTALL is causing SIGSEGV when someone is accessing >>> + * memory advised with it. >> Maybe add: >> * This is a test for feature implemented in >> * 662df3e5c376 ("mm: madvise: implement lightweight guard page mechanism") >> >> The rest LGTM. >> >> Reviewed-by: Petr Vorel <pvorel@suse.cz> >> >> Kind regards, >> Petr >> >>> + * >>> + * [Algorithm] >>> + * >>> + * - allocate a certain amount of memory >>> + * - advise memory with MADV_GUARD_INSTALL >>> + * - access to memory from within a child and verify it gets killed by SIGSEGV >>> + * - release memory with MADV_GUARD_REMOVE >>> + * - verify that memory has not been modified before child got killed >>> + * - modify memory within a new child >>> + * - verify that memory is accessable and child was not killed by SIGSEGV >>> + */ >>> + >>> +#include "tst_test.h" >>> +#include "lapi/mmap.h" >>> + >>> +#define MAP_SIZE (8 * TST_KB) >>> + >>> +static char *addr; >>> + >>> +static void run(void) >>> +{ >>> + pid_t pid; >>> + int status; >>> + >>> + memset(addr, 0, MAP_SIZE); >>> + >>> + TST_EXP_PASS(madvise(addr, MAP_SIZE, MADV_GUARD_INSTALL)); >>> + >>> + pid = SAFE_FORK(); >>> + if (!pid) { >>> + tst_res(TINFO, "Modifying memory content"); >>> + >>> + memset(addr, 'a', MAP_SIZE); >>> + exit(0); >>> + } >>> + >>> + SAFE_WAITPID(pid, &status, 0); >>> + >>> + if (WIFSIGNALED(status) && WTERMSIG(status) == SIGSEGV) >>> + tst_res(TPASS, "Child ended by SIGSEGV as expected"); >>> + else >>> + tst_res(TFAIL, "Child: %s", tst_strstatus(status)); >>> + >>> + TST_EXP_PASS(madvise(addr, MAP_SIZE, MADV_GUARD_REMOVE)); >>> + >>> + for (int i = 0; i < MAP_SIZE; i++) { >>> + if (addr[i] == 'a') { >>> + tst_res(TFAIL, "Memory content has been modified"); >>> + return; >>> + } >>> + } >>> + >>> + tst_res(TPASS, "Memory content didn't change"); >>> + >>> + pid = SAFE_FORK(); >>> + if (!pid) { >>> + tst_res(TINFO, "Modifying memory content"); >>> + >>> + memset(addr, 'b', MAP_SIZE); >>> + exit(0); >>> + } >>> + >>> + SAFE_WAITPID(pid, &status, 0); >>> + >>> + if (!WIFSIGNALED(status)) >>> + tst_res(TPASS, "Child ended without being signaled"); >>> + else >>> + tst_res(TFAIL, "Child ended with %s", tst_strstatus(status)); >>> +} >>> + >>> +static void setup(void) >>> +{ >>> + addr = SAFE_MMAP(NULL, MAP_SIZE, >>> + PROT_READ | PROT_WRITE, >>> + MAP_PRIVATE | MAP_ANONYMOUS, >>> + -1, 0); >>> +} >>> + >>> +static void cleanup(void) >>> +{ >>> + if (addr) >>> + SAFE_MUNMAP(addr, MAP_SIZE); >>> +} >>> + >>> +static struct tst_test test = { >>> + .test_all = run, >>> + .setup = setup, >>> + .cleanup = cleanup, >>> + .needs_root = 1, >>> + .forks_child = 1, >>> + .min_kver = "6.13", >>> +}; >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp
Hi Lorenzo, Andrea, > On Wed, Dec 11, 2024 at 01:30:48AM +0100, Petr Vorel wrote: > > Hi Andrea, > > [ I dared to Cc Lorenzo, the author of the kernel implementation ] > I'm not that scary am I? :))) > Feel free to cc- me on anything relating to this even if obviously in the > LTP project happy to be included! :) Thank you, we really appreciate that! > > > Verify that MADV_GUARD_INSTALL is causing SIGSEGV when someone is > > > trying to access memory advised with it. > > Test LGTM, thanks for covering a new kernel feature. > All good, looks fine to me, though might be worth expanding over time, we > have some self tests in the kernel for this, see > tools/testing/selftests/mm/guard-pages.c. Great! Noted at the ticket. > But it's nice to have a basic LTP regression test to assert the fundamental > thing is working as it should, and also nice that you implement it from > your perspective rather than mine, where I am obviously rather influenced > by implementation details. > Also note I submitted man pages for the change, you can pull it from [0] > and view it via: > $ man --manpath=. 2 madvise > [0]:git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git +1 Kind regards, Petr > Hopefully these will get distributed around soon! > Thanks for doing this for my feature, much appreciated overall! > Cheers, Lorenzo ...
diff --git a/runtest/syscalls b/runtest/syscalls index 5fd62617df1a116b1d94c57ff30f74693320a2ab..ded035ee82d0e97c67cc1e7c487b010634b2d1a0 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1000,6 +1000,7 @@ madvise08 madvise08 madvise09 madvise09 madvise10 madvise10 madvise11 madvise11 +madvise12 madvise12 newuname01 newuname01 diff --git a/testcases/kernel/syscalls/madvise/.gitignore b/testcases/kernel/syscalls/madvise/.gitignore index 722ac3c34306bac414313f1ce36ca98d715cd04c..758e601a9c4e7682a925f16184d14f2357009bc2 100644 --- a/testcases/kernel/syscalls/madvise/.gitignore +++ b/testcases/kernel/syscalls/madvise/.gitignore @@ -8,3 +8,4 @@ /madvise09 /madvise10 /madvise11 +/madvise12 diff --git a/testcases/kernel/syscalls/madvise/madvise12.c b/testcases/kernel/syscalls/madvise/madvise12.c new file mode 100644 index 0000000000000000000000000000000000000000..2bdf843f016a7c9d175a31b76ae805d63c4cbc80 --- /dev/null +++ b/testcases/kernel/syscalls/madvise/madvise12.c @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2024 Andrea Cervesato <andrea.cervesato@suse.com> + */ + +/*\ + * [Description] + * + * Verify that MADV_GUARD_INSTALL is causing SIGSEGV when someone is accessing + * memory advised with it. + * + * [Algorithm] + * + * - allocate a certain amount of memory + * - advise memory with MADV_GUARD_INSTALL + * - access to memory from within a child and verify it gets killed by SIGSEGV + * - release memory with MADV_GUARD_REMOVE + * - verify that memory has not been modified before child got killed + * - modify memory within a new child + * - verify that memory is accessable and child was not killed by SIGSEGV + */ + +#include "tst_test.h" +#include "lapi/mmap.h" + +#define MAP_SIZE (8 * TST_KB) + +static char *addr; + +static void run(void) +{ + pid_t pid; + int status; + + memset(addr, 0, MAP_SIZE); + + TST_EXP_PASS(madvise(addr, MAP_SIZE, MADV_GUARD_INSTALL)); + + pid = SAFE_FORK(); + if (!pid) { + tst_res(TINFO, "Modifying memory content"); + + memset(addr, 'a', MAP_SIZE); + exit(0); + } + + SAFE_WAITPID(pid, &status, 0); + + if (WIFSIGNALED(status) && WTERMSIG(status) == SIGSEGV) + tst_res(TPASS, "Child ended by SIGSEGV as expected"); + else + tst_res(TFAIL, "Child: %s", tst_strstatus(status)); + + TST_EXP_PASS(madvise(addr, MAP_SIZE, MADV_GUARD_REMOVE)); + + for (int i = 0; i < MAP_SIZE; i++) { + if (addr[i] == 'a') { + tst_res(TFAIL, "Memory content has been modified"); + return; + } + } + + tst_res(TPASS, "Memory content didn't change"); + + pid = SAFE_FORK(); + if (!pid) { + tst_res(TINFO, "Modifying memory content"); + + memset(addr, 'b', MAP_SIZE); + exit(0); + } + + SAFE_WAITPID(pid, &status, 0); + + if (!WIFSIGNALED(status)) + tst_res(TPASS, "Child ended without being signaled"); + else + tst_res(TFAIL, "Child ended with %s", tst_strstatus(status)); +} + +static void setup(void) +{ + addr = SAFE_MMAP(NULL, MAP_SIZE, + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, + -1, 0); +} + +static void cleanup(void) +{ + if (addr) + SAFE_MUNMAP(addr, MAP_SIZE); +} + +static struct tst_test test = { + .test_all = run, + .setup = setup, + .cleanup = cleanup, + .needs_root = 1, + .forks_child = 1, + .min_kver = "6.13", +};