* [PATCH] selftests: bpf: mmap: reorder mmap manipulations of adv_mmap tests
@ 2020-08-10 15:31 Yauheni Kaliuta
2020-08-10 19:12 ` Martin KaFai Lau
2020-08-11 0:26 ` Andrii Nakryiko
0 siblings, 2 replies; 5+ messages in thread
From: Yauheni Kaliuta @ 2020-08-10 15:31 UTC (permalink / raw
To: bpf; +Cc: Andrii Nakryiko, Daniel Borkmann
The idea of adv_mmap tests is to map/unmap pages in arbitrary
order. It works fine as soon as the kernel allocates first 3 pages
for from a region with unallocated page after that. If it's not the
case, the last remapping of 4 pages with MAP_FIXED will remap the
page to bpf map which will break the code which worked with the data
located there before.
Change the test to map first the whole bpf map, 4 pages, and then
manipulate the mappings.
Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
tools/testing/selftests/bpf/prog_tests/mmap.c | 23 ++++++++++++-------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/mmap.c b/tools/testing/selftests/bpf/prog_tests/mmap.c
index 43d0b5578f46..5768af1e16a7 100644
--- a/tools/testing/selftests/bpf/prog_tests/mmap.c
+++ b/tools/testing/selftests/bpf/prog_tests/mmap.c
@@ -183,38 +183,45 @@ void test_mmap(void)
/* check some more advanced mmap() manipulations */
- /* map all but last page: pages 1-3 mapped */
- tmp1 = mmap(NULL, 3 * page_size, PROT_READ, MAP_SHARED,
+ /* map all 4 pages */
+ tmp1 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED,
data_map_fd, 0);
if (CHECK(tmp1 == MAP_FAILED, "adv_mmap1", "errno %d\n", errno))
goto cleanup;
- /* unmap second page: pages 1, 3 mapped */
+ /* unmap second page: pages 1, 3, 4 mapped */
err = munmap(tmp1 + page_size, page_size);
if (CHECK(err, "adv_mmap2", "errno %d\n", errno)) {
munmap(tmp1, map_sz);
goto cleanup;
}
+ /* unmap forth page: pages 1, 3 mapped */
+ err = munmap(tmp1 + (3 * page_size), page_size);
+ if (CHECK(err, "adv_mmap3", "errno %d\n", errno)) {
+ munmap(tmp1, map_sz);
+ goto cleanup;
+ }
+
/* map page 2 back */
tmp2 = mmap(tmp1 + page_size, page_size, PROT_READ,
MAP_SHARED | MAP_FIXED, data_map_fd, 0);
- if (CHECK(tmp2 == MAP_FAILED, "adv_mmap3", "errno %d\n", errno)) {
+ if (CHECK(tmp2 == MAP_FAILED, "adv_mmap4", "errno %d\n", errno)) {
munmap(tmp1, page_size);
munmap(tmp1 + 2*page_size, page_size);
goto cleanup;
}
- CHECK(tmp1 + page_size != tmp2, "adv_mmap4",
+ CHECK(tmp1 + page_size != tmp2, "adv_mmap5",
"tmp1: %p, tmp2: %p\n", tmp1, tmp2);
/* re-map all 4 pages */
tmp2 = mmap(tmp1, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
data_map_fd, 0);
- if (CHECK(tmp2 == MAP_FAILED, "adv_mmap5", "errno %d\n", errno)) {
+ if (CHECK(tmp2 == MAP_FAILED, "adv_mmap6", "errno %d\n", errno)) {
munmap(tmp1, 3 * page_size); /* unmap page 1 */
goto cleanup;
}
- CHECK(tmp1 != tmp2, "adv_mmap6", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
+ CHECK(tmp1 != tmp2, "adv_mmap7", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
map_data = tmp2;
CHECK_FAIL(bss_data->in_val != 321);
@@ -231,7 +238,7 @@ void test_mmap(void)
/* map all 4 pages, but with pg_off=1 page, should fail */
tmp1 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
data_map_fd, page_size /* initial page shift */);
- if (CHECK(tmp1 != MAP_FAILED, "adv_mmap7", "unexpected success")) {
+ if (CHECK(tmp1 != MAP_FAILED, "adv_mmap8", "unexpected success")) {
munmap(tmp1, 4 * page_size);
goto cleanup;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] selftests: bpf: mmap: reorder mmap manipulations of adv_mmap tests
2020-08-10 15:31 [PATCH] selftests: bpf: mmap: reorder mmap manipulations of adv_mmap tests Yauheni Kaliuta
@ 2020-08-10 19:12 ` Martin KaFai Lau
2020-08-10 19:54 ` Yauheni Kaliuta
2020-08-11 0:26 ` Andrii Nakryiko
1 sibling, 1 reply; 5+ messages in thread
From: Martin KaFai Lau @ 2020-08-10 19:12 UTC (permalink / raw
To: Yauheni Kaliuta; +Cc: bpf, Andrii Nakryiko, Daniel Borkmann
On Mon, Aug 10, 2020 at 06:31:39PM +0300, Yauheni Kaliuta wrote:
> The idea of adv_mmap tests is to map/unmap pages in arbitrary
> order. It works fine as soon as the kernel allocates first 3 pages
> for from a region with unallocated page after that. If it's not the
> case, the last remapping of 4 pages with MAP_FIXED will remap the
> page to bpf map which will break the code which worked with the data
> located there before.
>
> Change the test to map first the whole bpf map, 4 pages, and then
> manipulate the mappings.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
> tools/testing/selftests/bpf/prog_tests/mmap.c | 23 ++++++++++++-------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/mmap.c b/tools/testing/selftests/bpf/prog_tests/mmap.c
> index 43d0b5578f46..5768af1e16a7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mmap.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mmap.c
> @@ -183,38 +183,45 @@ void test_mmap(void)
>
> /* check some more advanced mmap() manipulations */
>
> - /* map all but last page: pages 1-3 mapped */
> - tmp1 = mmap(NULL, 3 * page_size, PROT_READ, MAP_SHARED,
> + /* map all 4 pages */
> + tmp1 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED,
> data_map_fd, 0);
> if (CHECK(tmp1 == MAP_FAILED, "adv_mmap1", "errno %d\n", errno))
> goto cleanup;
>
> - /* unmap second page: pages 1, 3 mapped */
> + /* unmap second page: pages 1, 3, 4 mapped */
> err = munmap(tmp1 + page_size, page_size);
> if (CHECK(err, "adv_mmap2", "errno %d\n", errno)) {
> munmap(tmp1, map_sz);
> goto cleanup;
> }
>
> + /* unmap forth page: pages 1, 3 mapped */
> + err = munmap(tmp1 + (3 * page_size), page_size);
> + if (CHECK(err, "adv_mmap3", "errno %d\n", errno)) {
> + munmap(tmp1, map_sz);
1, 3, and 4 are mapped here but only one munmap() with "map_sz" is used.
> + goto cleanup;
> + }
> +
> /* map page 2 back */
> tmp2 = mmap(tmp1 + page_size, page_size, PROT_READ,
> MAP_SHARED | MAP_FIXED, data_map_fd, 0);
> - if (CHECK(tmp2 == MAP_FAILED, "adv_mmap3", "errno %d\n", errno)) {
> + if (CHECK(tmp2 == MAP_FAILED, "adv_mmap4", "errno %d\n", errno)) {
> munmap(tmp1, page_size);
> munmap(tmp1 + 2*page_size, page_size);
1 and 3 are mapped here. However, multiple munmap() are used.
Both will work the same?
> goto cleanup;
> }
> - CHECK(tmp1 + page_size != tmp2, "adv_mmap4",
> + CHECK(tmp1 + page_size != tmp2, "adv_mmap5",
> "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
>
> /* re-map all 4 pages */
> tmp2 = mmap(tmp1, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
> data_map_fd, 0);
> - if (CHECK(tmp2 == MAP_FAILED, "adv_mmap5", "errno %d\n", errno)) {
> + if (CHECK(tmp2 == MAP_FAILED, "adv_mmap6", "errno %d\n", errno)) {
> munmap(tmp1, 3 * page_size); /* unmap page 1 */
> goto cleanup;
> }
> - CHECK(tmp1 != tmp2, "adv_mmap6", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
> + CHECK(tmp1 != tmp2, "adv_mmap7", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
>
> map_data = tmp2;
> CHECK_FAIL(bss_data->in_val != 321);
> @@ -231,7 +238,7 @@ void test_mmap(void)
> /* map all 4 pages, but with pg_off=1 page, should fail */
> tmp1 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
> data_map_fd, page_size /* initial page shift */);
> - if (CHECK(tmp1 != MAP_FAILED, "adv_mmap7", "unexpected success")) {
> + if (CHECK(tmp1 != MAP_FAILED, "adv_mmap8", "unexpected success")) {
> munmap(tmp1, 4 * page_size);
> goto cleanup;
> }
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selftests: bpf: mmap: reorder mmap manipulations of adv_mmap tests
2020-08-10 19:12 ` Martin KaFai Lau
@ 2020-08-10 19:54 ` Yauheni Kaliuta
0 siblings, 0 replies; 5+ messages in thread
From: Yauheni Kaliuta @ 2020-08-10 19:54 UTC (permalink / raw
To: Martin KaFai Lau; +Cc: bpf, Andrii Nakryiko, Daniel Borkmann
On Mon, Aug 10, 2020 at 10:13 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Mon, Aug 10, 2020 at 06:31:39PM +0300, Yauheni Kaliuta wrote:
> > The idea of adv_mmap tests is to map/unmap pages in arbitrary
> > order. It works fine as soon as the kernel allocates first 3 pages
> > for from a region with unallocated page after that. If it's not the
> > case, the last remapping of 4 pages with MAP_FIXED will remap the
> > page to bpf map which will break the code which worked with the data
> > located there before.
> >
> > Change the test to map first the whole bpf map, 4 pages, and then
> > manipulate the mappings.
> >
> > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> > ---
> > tools/testing/selftests/bpf/prog_tests/mmap.c | 23 ++++++++++++-------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/mmap.c b/tools/testing/selftests/bpf/prog_tests/mmap.c
> > index 43d0b5578f46..5768af1e16a7 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/mmap.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/mmap.c
> > @@ -183,38 +183,45 @@ void test_mmap(void)
> >
> > /* check some more advanced mmap() manipulations */
> >
> > - /* map all but last page: pages 1-3 mapped */
> > - tmp1 = mmap(NULL, 3 * page_size, PROT_READ, MAP_SHARED,
> > + /* map all 4 pages */
> > + tmp1 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED,
> > data_map_fd, 0);
> > if (CHECK(tmp1 == MAP_FAILED, "adv_mmap1", "errno %d\n", errno))
> > goto cleanup;
> >
> > - /* unmap second page: pages 1, 3 mapped */
> > + /* unmap second page: pages 1, 3, 4 mapped */
> > err = munmap(tmp1 + page_size, page_size);
> > if (CHECK(err, "adv_mmap2", "errno %d\n", errno)) {
> > munmap(tmp1, map_sz);
> > goto cleanup;
> > }
> >
> > + /* unmap forth page: pages 1, 3 mapped */
> > + err = munmap(tmp1 + (3 * page_size), page_size);
> > + if (CHECK(err, "adv_mmap3", "errno %d\n", errno)) {
> > + munmap(tmp1, map_sz);
> 1, 3, and 4 are mapped here but only one munmap() with "map_sz" is used.
Actually works for me:
7ffff7dd4000-7ffff7dfd000 r-xp 00000000 fd:05 3147480
/usr/lib64/ld-2.28.so
7ffff7fe3000-7ffff7fe4000 r--s 00000000 00:0e 10298
anon_inode:bpf-map
7ffff7fe5000-7ffff7fe7000 r--s 00002000 00:0e 10298
anon_inode:bpf-map
7ffff7fe7000-7ffff7fee000 rw-p 00000000 00:00 0
7ffff7ff1000-7ffff7ff5000 r--s 00000000 00:0e 10298
anon_inode:bpf-map
After such unmap:
7ffff7dd4000-7ffff7dfd000 r-xp 00000000 fd:05 3147480
/usr/lib64/ld-2.28.so
7ffff7fe7000-7ffff7fee000 rw-p 00000000 00:00 0
7ffff7ff1000-7ffff7ff5000 r--s 00000000 00:0e 10298
anon_inode:bpf-map
>
> > + goto cleanup;
> > + }
> > +
> > /* map page 2 back */
> > tmp2 = mmap(tmp1 + page_size, page_size, PROT_READ,
> > MAP_SHARED | MAP_FIXED, data_map_fd, 0);
> > - if (CHECK(tmp2 == MAP_FAILED, "adv_mmap3", "errno %d\n", errno)) {
> > + if (CHECK(tmp2 == MAP_FAILED, "adv_mmap4", "errno %d\n", errno)) {
> > munmap(tmp1, page_size);
> > munmap(tmp1 + 2*page_size, page_size);
> 1 and 3 are mapped here. However, multiple munmap() are used.
>
> Both will work the same?
For the case when we do not care about the already unmapped page.
But may be I should unify and do it the same how it was done before.
Thanks!
>
> > goto cleanup;
> > }
> > - CHECK(tmp1 + page_size != tmp2, "adv_mmap4",
> > + CHECK(tmp1 + page_size != tmp2, "adv_mmap5",
> > "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
> >
> > /* re-map all 4 pages */
> > tmp2 = mmap(tmp1, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
> > data_map_fd, 0);
> > - if (CHECK(tmp2 == MAP_FAILED, "adv_mmap5", "errno %d\n", errno)) {
> > + if (CHECK(tmp2 == MAP_FAILED, "adv_mmap6", "errno %d\n", errno)) {
> > munmap(tmp1, 3 * page_size); /* unmap page 1 */
> > goto cleanup;
> > }
> > - CHECK(tmp1 != tmp2, "adv_mmap6", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
> > + CHECK(tmp1 != tmp2, "adv_mmap7", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
> >
> > map_data = tmp2;
> > CHECK_FAIL(bss_data->in_val != 321);
> > @@ -231,7 +238,7 @@ void test_mmap(void)
> > /* map all 4 pages, but with pg_off=1 page, should fail */
> > tmp1 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
> > data_map_fd, page_size /* initial page shift */);
> > - if (CHECK(tmp1 != MAP_FAILED, "adv_mmap7", "unexpected success")) {
> > + if (CHECK(tmp1 != MAP_FAILED, "adv_mmap8", "unexpected success")) {
> > munmap(tmp1, 4 * page_size);
> > goto cleanup;
> > }
> > --
> > 2.26.2
> >
>
--
WBR, Yauheni
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selftests: bpf: mmap: reorder mmap manipulations of adv_mmap tests
2020-08-10 15:31 [PATCH] selftests: bpf: mmap: reorder mmap manipulations of adv_mmap tests Yauheni Kaliuta
2020-08-10 19:12 ` Martin KaFai Lau
@ 2020-08-11 0:26 ` Andrii Nakryiko
2020-08-11 4:38 ` Yauheni Kaliuta
1 sibling, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2020-08-11 0:26 UTC (permalink / raw
To: Yauheni Kaliuta; +Cc: bpf, Andrii Nakryiko, Daniel Borkmann
On Mon, Aug 10, 2020 at 8:32 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> The idea of adv_mmap tests is to map/unmap pages in arbitrary
> order. It works fine as soon as the kernel allocates first 3 pages
> for from a region with unallocated page after that. If it's not the
> case, the last remapping of 4 pages with MAP_FIXED will remap the
> page to bpf map which will break the code which worked with the data
> located there before.
>
> Change the test to map first the whole bpf map, 4 pages, and then
> manipulate the mappings.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
[0] is fixing the same issue with a slightly different approach by
"preallocating" 4 anonymous mmap pages. I think I like that one a bit
better. Please take a look as well.
[0] https://patchwork.ozlabs.org/project/netdev/patch/20200810153940.125508-1-Jianlin.Lv@arm.com/
> tools/testing/selftests/bpf/prog_tests/mmap.c | 23 ++++++++++++-------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selftests: bpf: mmap: reorder mmap manipulations of adv_mmap tests
2020-08-11 0:26 ` Andrii Nakryiko
@ 2020-08-11 4:38 ` Yauheni Kaliuta
0 siblings, 0 replies; 5+ messages in thread
From: Yauheni Kaliuta @ 2020-08-11 4:38 UTC (permalink / raw
To: Andrii Nakryiko; +Cc: bpf, Andrii Nakryiko, Daniel Borkmann
On Tue, Aug 11, 2020 at 3:26 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Aug 10, 2020 at 8:32 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
> >
> > The idea of adv_mmap tests is to map/unmap pages in arbitrary
> > order. It works fine as soon as the kernel allocates first 3 pages
> > for from a region with unallocated page after that. If it's not the
> > case, the last remapping of 4 pages with MAP_FIXED will remap the
> > page to bpf map which will break the code which worked with the data
> > located there before.
> >
> > Change the test to map first the whole bpf map, 4 pages, and then
> > manipulate the mappings.
> >
> > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> > ---
>
> [0] is fixing the same issue with a slightly different approach by
> "preallocating" 4 anonymous mmap pages. I think I like that one a bit
> better. Please take a look as well.
Fine for me. Thanks!
>
> [0] https://patchwork.ozlabs.org/project/netdev/patch/20200810153940.125508-1-Jianlin.Lv@arm.com/
>
> > tools/testing/selftests/bpf/prog_tests/mmap.c | 23 ++++++++++++-------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >
>
> [...]
>
--
WBR, Yauheni
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-11 4:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-10 15:31 [PATCH] selftests: bpf: mmap: reorder mmap manipulations of adv_mmap tests Yauheni Kaliuta
2020-08-10 19:12 ` Martin KaFai Lau
2020-08-10 19:54 ` Yauheni Kaliuta
2020-08-11 0:26 ` Andrii Nakryiko
2020-08-11 4:38 ` Yauheni Kaliuta
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.