bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* selftests: bpf: mmap question
@ 2020-07-23 11:02 Yauheni Kaliuta
  2020-07-28  5:15 ` Andrii Nakryiko
  0 siblings, 1 reply; 4+ messages in thread
From: Yauheni Kaliuta @ 2020-07-23 11:02 UTC (permalink / raw)
  To: bpf; +Cc: Andrii Nakryiko, Jiri Olsa, Jiri Benc

Hi!

I have a question about the part of the test:

	/* 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,
			  data_map_fd, 0);
	if (CHECK(tmp1 == MAP_FAILED, "adv_mmap1", "errno %d\n", errno))
		goto cleanup;

	/* unmap second page: pages 1, 3 mapped */
	err = munmap(tmp1 + page_size, page_size);
	if (CHECK(err, "adv_mmap2", "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)) {
		munmap(tmp1, page_size);
		munmap(tmp1 + 2*page_size, page_size);
		goto cleanup;
	}
	CHECK(tmp1 + page_size != tmp2, "adv_mmap4",
	      "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)) {
		munmap(tmp1, 3 * page_size); /* unmap page 1 */
		goto cleanup;
	}
	CHECK(tmp1 != tmp2, "adv_mmap6", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);


In my configuration the first mapping

	/* map all but last page: pages 1-3 mapped */
	tmp1 = mmap(NULL, 3 * page_size, PROT_READ, MAP_SHARED,
			  data_map_fd, 0);


maps the area to the 3 pages right before the TLS page.
I find it's pretty ok.

But then the 4 page mapping

	/* re-map all 4 pages */
	tmp2 = mmap(tmp1, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
		    data_map_fd, 0);


since it has MAP_FIXED flag, unmaps TLS and maps the former TLS
address BPF map.

Which is again exactly the behaviour of MAP_FIXED, but it breaks
the test.

Using MAP_FIXED_NOREPLACE fails the check:

CHECK(tmp1 != tmp2, "adv_mmap6", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);

as expected.


Should the test be modified to be a bit more relaxed? Since the
kernel behaviour looks correct or I'm missing something?


PS: BTW, the previous data_map mapping left unmmaped. Is it expected?

-- 
WBR,
Yauheni Kaliuta


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: selftests: bpf: mmap question
  2020-07-23 11:02 selftests: bpf: mmap question Yauheni Kaliuta
@ 2020-07-28  5:15 ` Andrii Nakryiko
  2020-08-10 15:31   ` Yauheni Kaliuta
  0 siblings, 1 reply; 4+ messages in thread
From: Andrii Nakryiko @ 2020-07-28  5:15 UTC (permalink / raw)
  To: Yauheni Kaliuta; +Cc: bpf, Andrii Nakryiko, Jiri Olsa, Jiri Benc

On Thu, Jul 23, 2020 at 4:02 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi!
>
> I have a question about the part of the test:
>

[...]

>
> In my configuration the first mapping
>
>         /* map all but last page: pages 1-3 mapped */
>         tmp1 = mmap(NULL, 3 * page_size, PROT_READ, MAP_SHARED,
>                           data_map_fd, 0);
>
>
> maps the area to the 3 pages right before the TLS page.
> I find it's pretty ok.

Hm... I never ran into this problem. The point here is to be able to
re-mmap partial ranges. One way would be to re-write all those
manipulations to start with a full range map, and then do partial
un-mmaps/re-mmaps, eventually just re-mmaping everything back. I think
that would work, right, as long as we never unmmap the last page? Do
you mind trying to fix the test in such a fashion?

>
> But then the 4 page mapping
>
>         /* re-map all 4 pages */
>         tmp2 = mmap(tmp1, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
>                     data_map_fd, 0);
>
>
> since it has MAP_FIXED flag, unmaps TLS and maps the former TLS
> address BPF map.
>
> Which is again exactly the behaviour of MAP_FIXED, but it breaks
> the test.
>
> Using MAP_FIXED_NOREPLACE fails the check:
>
> CHECK(tmp1 != tmp2, "adv_mmap6", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
>
> as expected.
>
>
> Should the test be modified to be a bit more relaxed? Since the
> kernel behaviour looks correct or I'm missing something?
>
>
> PS: BTW, the previous data_map mapping left unmmaped. Is it expected?

Not intentional, the idea is that each test exits with a clean state.

>
> --
> WBR,
> Yauheni Kaliuta
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: selftests: bpf: mmap question
  2020-07-28  5:15 ` Andrii Nakryiko
@ 2020-08-10 15:31   ` Yauheni Kaliuta
  2020-08-11  0:27     ` Andrii Nakryiko
  0 siblings, 1 reply; 4+ messages in thread
From: Yauheni Kaliuta @ 2020-08-10 15:31 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, Andrii Nakryiko, Jiri Olsa, Jiri Benc

Hi!

On Tue, Jul 28, 2020 at 8:15 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jul 23, 2020 at 4:02 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
> >
> > Hi!
> >
> > I have a question about the part of the test:
> >
>
> [...]
>
> >
> > In my configuration the first mapping
> >
> >         /* map all but last page: pages 1-3 mapped */
> >         tmp1 = mmap(NULL, 3 * page_size, PROT_READ, MAP_SHARED,
> >                           data_map_fd, 0);
> >
> >
> > maps the area to the 3 pages right before the TLS page.
> > I find it's pretty ok.
>
> Hm... I never ran into this problem. The point here is to be able to
> re-mmap partial ranges. One way would be to re-write all those
> manipulations to start with a full range map, and then do partial
> un-mmaps/re-mmaps, eventually just re-mmaping everything back. I think
> that would work, right, as long as we never unmmap the last page? Do
> you mind trying to fix the test in such a fashion?

Sorry for the delay. I don't sure, sending the patch.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: selftests: bpf: mmap question
  2020-08-10 15:31   ` Yauheni Kaliuta
@ 2020-08-11  0:27     ` Andrii Nakryiko
  0 siblings, 0 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2020-08-11  0:27 UTC (permalink / raw)
  To: Yauheni Kaliuta; +Cc: bpf, Andrii Nakryiko, Jiri Olsa, Jiri Benc

On Mon, Aug 10, 2020 at 8:31 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi!
>
> On Tue, Jul 28, 2020 at 8:15 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Jul 23, 2020 at 4:02 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> > >
> > > Hi!
> > >
> > > I have a question about the part of the test:
> > >
> >
> > [...]
> >
> > >
> > > In my configuration the first mapping
> > >
> > >         /* map all but last page: pages 1-3 mapped */
> > >         tmp1 = mmap(NULL, 3 * page_size, PROT_READ, MAP_SHARED,
> > >                           data_map_fd, 0);
> > >
> > >
> > > maps the area to the 3 pages right before the TLS page.
> > > I find it's pretty ok.
> >
> > Hm... I never ran into this problem. The point here is to be able to
> > re-mmap partial ranges. One way would be to re-write all those
> > manipulations to start with a full range map, and then do partial
> > un-mmaps/re-mmaps, eventually just re-mmaping everything back. I think
> > that would work, right, as long as we never unmmap the last page? Do
> > you mind trying to fix the test in such a fashion?
>
> Sorry for the delay. I don't sure, sending the patch.
>

No worries! Meanwhile there has been an alternative fix for the same bug: [0]!

  [0] https://patchwork.ozlabs.org/project/netdev/patch/20200810153940.125508-1-Jianlin.Lv@arm.com/

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-08-11  0:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23 11:02 selftests: bpf: mmap question Yauheni Kaliuta
2020-07-28  5:15 ` Andrii Nakryiko
2020-08-10 15:31   ` Yauheni Kaliuta
2020-08-11  0:27     ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).