bpf selftests and page size

bpf selftests and page size

[Yauheni Kaliuta]

Hi!

Bunch of bpf selftests actually depends of page size and has it
hardcoded to 4K. That causes failures if page shift is configured
to values other than 12. It looks as a known issue since for the
userspace parts sysconf(_SC_PAGE_SIZE) is used, but what would be
the correct way to export it to bpf programs?

-- 
WBR,
Yauheni Kaliuta

Re: bpf selftests and page size

[Andrii Nakryiko]

On Mon, Mar 1, 2021 at 1:02 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi!
>
> Bunch of bpf selftests actually depends of page size and has it
> hardcoded to 4K. That causes failures if page shift is configured
> to values other than 12. It looks as a known issue since for the
> userspace parts sysconf(_SC_PAGE_SIZE) is used, but what would be
> the correct way to export it to bpf programs?
>

Given PAGE_SIZE and PAGE_SHIFT are just #defines, the only way seems
to be to pass it from the user-space as a read-only variable.

> --
> WBR,
> Yauheni Kaliuta
>

Re: bpf selftests and page size

[Yauheni Kaliuta]

Hi!

On Tue, Mar 2, 2021 at 7:08 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Mar 1, 2021 at 1:02 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
>
> > Bunch of bpf selftests actually depends of page size and has it
> > hardcoded to 4K. That causes failures if page shift is configured
> > to values other than 12. It looks as a known issue since for the
> > userspace parts sysconf(_SC_PAGE_SIZE) is used, but what would be
> > the correct way to export it to bpf programs?
> >
>
> Given PAGE_SIZE and PAGE_SHIFT are just #defines, the only way seems
> to be to pass it from the user-space as a read-only variable.

Compile-time? Just to make sure we are on the same page, the tests may look like
https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/progs/map_ptr_kern.c#L638


-- 
WBR, Yauheni

Re: bpf selftests and page size

[Andrii Nakryiko]

On Wed, Mar 3, 2021 at 2:40 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi!
>
> On Tue, Mar 2, 2021 at 7:08 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Mar 1, 2021 at 1:02 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> >
> > > Bunch of bpf selftests actually depends of page size and has it
> > > hardcoded to 4K. That causes failures if page shift is configured
> > > to values other than 12. It looks as a known issue since for the
> > > userspace parts sysconf(_SC_PAGE_SIZE) is used, but what would be
> > > the correct way to export it to bpf programs?
> > >
> >
> > Given PAGE_SIZE and PAGE_SHIFT are just #defines, the only way seems
> > to be to pass it from the user-space as a read-only variable.
>
> Compile-time? Just to make sure we are on the same page, the tests may look like
> https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/progs/map_ptr_kern.c#L638
>

Oh, if it's about ringbuf, then why not just bump its size to 64KB or
something even bigger. I thought that you were referring to some BPF
code that needs to do some calculations based on PAGE_SIZE in runtime.

>
> --
> WBR, Yauheni
>

Re: bpf selftests and page size

[Yauheni Kaliuta]

Hi!

On Wed, Mar 3, 2021 at 7:30 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Mar 3, 2021 at 2:40 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
> >
> > Hi!
> >
> > On Tue, Mar 2, 2021 at 7:08 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Mar 1, 2021 at 1:02 AM Yauheni Kaliuta
> > > <yauheni.kaliuta@redhat.com> wrote:
> > >
> > > > Bunch of bpf selftests actually depends of page size and has it
> > > > hardcoded to 4K. That causes failures if page shift is configured
> > > > to values other than 12. It looks as a known issue since for the
> > > > userspace parts sysconf(_SC_PAGE_SIZE) is used, but what would be
> > > > the correct way to export it to bpf programs?
> > > >
> > >
> > > Given PAGE_SIZE and PAGE_SHIFT are just #defines, the only way seems
> > > to be to pass it from the user-space as a read-only variable.
> >
> > Compile-time? Just to make sure we are on the same page, the tests may look like
> > https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/progs/map_ptr_kern.c#L638
> >
>
> Oh, if it's about ringbuf, then why not just bump its size to 64KB or
> something even bigger.

It will work for me in this, but sounds as workaround since the value
depends of actual page size.

>
> I thought that you were referring to some BPF code that needs to do some calculations based on PAGE_SIZE in runtime.

Do you mean, like that
https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/progs/sockopt_sk.c#L12
?

There are such tests as well  :)

Re: bpf selftests and page size

[Andrii Nakryiko]

On Thu, Mar 4, 2021 at 12:35 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi!
>
> On Wed, Mar 3, 2021 at 7:30 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Mar 3, 2021 at 2:40 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> > >
> > > Hi!
> > >
> > > On Tue, Mar 2, 2021 at 7:08 AM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 1, 2021 at 1:02 AM Yauheni Kaliuta
> > > > <yauheni.kaliuta@redhat.com> wrote:
> > > >
> > > > > Bunch of bpf selftests actually depends of page size and has it
> > > > > hardcoded to 4K. That causes failures if page shift is configured
> > > > > to values other than 12. It looks as a known issue since for the
> > > > > userspace parts sysconf(_SC_PAGE_SIZE) is used, but what would be
> > > > > the correct way to export it to bpf programs?
> > > > >
> > > >
> > > > Given PAGE_SIZE and PAGE_SHIFT are just #defines, the only way seems
> > > > to be to pass it from the user-space as a read-only variable.
> > >
> > > Compile-time? Just to make sure we are on the same page, the tests may look like
> > > https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/progs/map_ptr_kern.c#L638
> > >
> >
> > Oh, if it's about ringbuf, then why not just bump its size to 64KB or
> > something even bigger.
>
> It will work for me in this, but sounds as workaround since the value
> depends of actual page size.

Not at all a work around. ringbuf needs to have the size which is a
multiple of PAGE_SIZE.

>
> >
> > I thought that you were referring to some BPF code that needs to do some calculations based on PAGE_SIZE in runtime.
>
> Do you mean, like that
> https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/progs/sockopt_sk.c#L12
> ?

yeah, here we'll need to pass actual page_size from the user-space
using global variable

>
> There are such tests as well  :)
>

Re: bpf selftests and page size

[Yauheni Kaliuta]

Hi, Andrii!

On Tue, Mar 2, 2021 at 7:08 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Mar 1, 2021 at 1:02 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
> >
> > Hi!
> >
> > Bunch of bpf selftests actually depends of page size and has it
> > hardcoded to 4K. That causes failures if page shift is configured
> > to values other than 12. It looks as a known issue since for the
> > userspace parts sysconf(_SC_PAGE_SIZE) is used, but what would be
> > the correct way to export it to bpf programs?
> >
>
> Given PAGE_SIZE and PAGE_SHIFT are just #defines, the only way seems
> to be to pass it from the user-space as a read-only variable.
>

I could not find a good example to attach to cgroup. Here is the
draft, could you point me to right direction?

diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
index d5b44b135c00..7932236a021e 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
@@ -1,8 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
 #include "cgroup_helpers.h"
-
 #include <linux/tcp.h>
+#include "sockopt_sk.skel.h"

 #ifndef SOL_TCP
 #define SOL_TCP IPPROTO_TCP
@@ -191,60 +191,33 @@ static int getsetsockopt(void)
     return -1;
 }

-static int prog_attach(struct bpf_object *obj, int cgroup_fd, const
char *title)
-{
-    enum bpf_attach_type attach_type;
-    enum bpf_prog_type prog_type;
-    struct bpf_program *prog;
-    int err;
-
-    err = libbpf_prog_type_by_name(title, &prog_type, &attach_type);
-    if (err) {
-        log_err("Failed to deduct types for %s BPF program", title);
-        return -1;
-    }
-
-    prog = bpf_object__find_program_by_title(obj, title);
-    if (!prog) {
-        log_err("Failed to find %s BPF program", title);
-        return -1;
-    }
-
-    err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
-                  attach_type, 0);
-    if (err) {
-        log_err("Failed to attach %s BPF program", title);
-        return -1;
-    }
-
-    return 0;
-}
-
 static void run_test(int cgroup_fd)
 {
-    struct bpf_prog_load_attr attr = {
-        .file = "./sockopt_sk.o",
-    };
-    struct bpf_object *obj;
-    int ignored;
+    struct sockopt_sk *skel;
+    int prog_fd;
+    int duration = 0;
     int err;

-    err = bpf_prog_load_xattr(&attr, &obj, &ignored);
-    if (CHECK_FAIL(err))
-        return;
+    skel = sockopt_sk__open_and_load();
+    if (CHECK(!skel, "skel_load", "sockopt_sk skeleton failed\n"))
+        goto cleanup;
+
+    skel->bss->page_size = getpagesize();

-    err = prog_attach(obj, cgroup_fd, "cgroup/getsockopt");
-    if (CHECK_FAIL(err))
-        goto close_bpf_object;
+    prog_fd = bpf_program__fd(skel->progs._getsockopt);
+    err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_GETSOCKOPT, 0);
+    if (CHECK(err, "attach", "getsockopt attach failed: %d\n", err))
+        goto cleanup;

-    err = prog_attach(obj, cgroup_fd, "cgroup/setsockopt");
-    if (CHECK_FAIL(err))
-        goto close_bpf_object;
+    prog_fd = bpf_program__fd(skel->progs._setsockopt);
+    err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SETSOCKOPT, 0);
+    if (CHECK(err, "attach", "setsockopt attach failed: %d\n", err))
+        goto cleanup;

     CHECK_FAIL(getsetsockopt());

-close_bpf_object:
-    bpf_object__close(obj);
+cleanup:
+    sockopt_sk__destroy(skel);
 }

 void test_sockopt_sk(void)
diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c
b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index d3597f81e6e9..f8b051589681 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -8,9 +8,7 @@
 char _license[] SEC("license") = "GPL";
 __u32 _version SEC("version") = 1;

-#ifndef PAGE_SIZE
-#define PAGE_SIZE 4096
-#endif
+int page_size; /* userspace should set it */

 #ifndef SOL_TCP
 #define SOL_TCP IPPROTO_TCP
@@ -41,7 +39,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
          * let next BPF program in the cgroup chain or kernel
          * handle it.
          */
-        ctx->optlen = 0; /* bypass optval>PAGE_SIZE */
+        ctx->optlen = 0; /* bypass optval>page_size */
         return 1;
     }

@@ -86,11 +84,11 @@ int _getsockopt(struct bpf_sockopt *ctx)
         optval[0] = 0x55;
         ctx->optlen = 1;

-        /* Userspace buffer is PAGE_SIZE * 2, but BPF
-         * program can only see the first PAGE_SIZE
+        /* Userspace buffer is page_size * 2, but BPF
+         * program can only see the first page_size
          * bytes of data.
          */
-        if (optval_end - optval != PAGE_SIZE)
+        if (optval_end - optval != page_size)
             return 0; /* EPERM, unexpected data size */

         return 1;
@@ -131,7 +129,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
          * let next BPF program in the cgroup chain or kernel
          * handle it.
          */
-        ctx->optlen = 0; /* bypass optval>PAGE_SIZE */
+        ctx->optlen = 0; /* bypass optval>page_size */
         return 1;
     }

@@ -160,8 +158,8 @@ int _setsockopt(struct bpf_sockopt *ctx)
     }

     if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
-        /* Original optlen is larger than PAGE_SIZE. */
-        if (ctx->optlen != PAGE_SIZE * 2)
+        /* Original optlen is larger than page_size. */
+        if (ctx->optlen != page_size * 2)
             return 0; /* EPERM, unexpected data size */

         if (optval + 1 > optval_end)
@@ -171,11 +169,11 @@ int _setsockopt(struct bpf_sockopt *ctx)
         optval[0] = 0;
         ctx->optlen = 1;

-        /* Usepace buffer is PAGE_SIZE * 2, but BPF
-         * program can only see the first PAGE_SIZE
+        /* Usepace buffer is page_size * 2, but BPF
+         * program can only see the first page_size
          * bytes of data.
          */
-        if (optval_end - optval != PAGE_SIZE)
+        if (optval_end - optval != page_size)
             return 0; /* EPERM, unexpected data size */

         return 1;


-- 
WBR, Yauheni

Re: bpf selftests and page size

[Andrii Nakryiko]

On Wed, Mar 24, 2021 at 6:28 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Andrii!
>
> On Tue, Mar 2, 2021 at 7:08 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Mar 1, 2021 at 1:02 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> > >
> > > Hi!
> > >
> > > Bunch of bpf selftests actually depends of page size and has it
> > > hardcoded to 4K. That causes failures if page shift is configured
> > > to values other than 12. It looks as a known issue since for the
> > > userspace parts sysconf(_SC_PAGE_SIZE) is used, but what would be
> > > the correct way to export it to bpf programs?
> > >
> >
> > Given PAGE_SIZE and PAGE_SHIFT are just #defines, the only way seems
> > to be to pass it from the user-space as a read-only variable.
> >
>
> I could not find a good example to attach to cgroup. Here is the
> draft, could you point me to right direction?

Yes. See prog_tests/cgroup_link.c, but I showed an example below.

>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> index d5b44b135c00..7932236a021e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> @@ -1,8 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <test_progs.h>
>  #include "cgroup_helpers.h"
> -
>  #include <linux/tcp.h>
> +#include "sockopt_sk.skel.h"
>
>  #ifndef SOL_TCP
>  #define SOL_TCP IPPROTO_TCP
> @@ -191,60 +191,33 @@ static int getsetsockopt(void)
>      return -1;
>  }
>
> -static int prog_attach(struct bpf_object *obj, int cgroup_fd, const
> char *title)
> -{
> -    enum bpf_attach_type attach_type;
> -    enum bpf_prog_type prog_type;
> -    struct bpf_program *prog;
> -    int err;
> -
> -    err = libbpf_prog_type_by_name(title, &prog_type, &attach_type);
> -    if (err) {
> -        log_err("Failed to deduct types for %s BPF program", title);
> -        return -1;
> -    }
> -
> -    prog = bpf_object__find_program_by_title(obj, title);
> -    if (!prog) {
> -        log_err("Failed to find %s BPF program", title);
> -        return -1;
> -    }
> -
> -    err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
> -                  attach_type, 0);
> -    if (err) {
> -        log_err("Failed to attach %s BPF program", title);
> -        return -1;
> -    }
> -
> -    return 0;
> -}
> -
>  static void run_test(int cgroup_fd)
>  {
> -    struct bpf_prog_load_attr attr = {
> -        .file = "./sockopt_sk.o",
> -    };
> -    struct bpf_object *obj;
> -    int ignored;
> +    struct sockopt_sk *skel;
> +    int prog_fd;
> +    int duration = 0;
>      int err;
>
> -    err = bpf_prog_load_xattr(&attr, &obj, &ignored);
> -    if (CHECK_FAIL(err))
> -        return;
> +    skel = sockopt_sk__open_and_load();
> +    if (CHECK(!skel, "skel_load", "sockopt_sk skeleton failed\n"))
> +        goto cleanup;
> +
> +    skel->bss->page_size = getpagesize();
>
> -    err = prog_attach(obj, cgroup_fd, "cgroup/getsockopt");
> -    if (CHECK_FAIL(err))
> -        goto close_bpf_object;
> +    prog_fd = bpf_program__fd(skel->progs._getsockopt);
> +    err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_GETSOCKOPT, 0);
> +    if (CHECK(err, "attach", "getsockopt attach failed: %d\n", err))
> +        goto cleanup;
>
> -    err = prog_attach(obj, cgroup_fd, "cgroup/setsockopt");
> -    if (CHECK_FAIL(err))
> -        goto close_bpf_object;
> +    prog_fd = bpf_program__fd(skel->progs._setsockopt);
> +    err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SETSOCKOPT, 0);

skel->links._setsockopt =
bpf_program__attach_cgroup(skel->progs._setsockopt, cgroup_fd);

same above for getsockopt

> +    if (CHECK(err, "attach", "setsockopt attach failed: %d\n", err))

nit: if (!ASSERT_OK(err, "setsockopt_attach"))

> +        goto cleanup;
>
>      CHECK_FAIL(getsetsockopt());
>
> -close_bpf_object:
> -    bpf_object__close(obj);
> +cleanup:
> +    sockopt_sk__destroy(skel);
>  }
>
>  void test_sockopt_sk(void)
> diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c
> b/tools/testing/selftests/bpf/progs/sockopt_sk.c
> index d3597f81e6e9..f8b051589681 100644
> --- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
> @@ -8,9 +8,7 @@
>  char _license[] SEC("license") = "GPL";
>  __u32 _version SEC("version") = 1;

while you are at it, please remove _version, it's useless now

>
> -#ifndef PAGE_SIZE
> -#define PAGE_SIZE 4096
> -#endif
> +int page_size; /* userspace should set it */
>
>  #ifndef SOL_TCP
>  #define SOL_TCP IPPROTO_TCP
> @@ -41,7 +39,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
>           * let next BPF program in the cgroup chain or kernel
>           * handle it.
>           */
> -        ctx->optlen = 0; /* bypass optval>PAGE_SIZE */
> +        ctx->optlen = 0; /* bypass optval>page_size */

you don't need to update all those comments, conceptually it's all
PAGE_SIZE. It's just distracting from the actual change in the patch.

>          return 1;
>      }
>
> @@ -86,11 +84,11 @@ int _getsockopt(struct bpf_sockopt *ctx)
>          optval[0] = 0x55;
>          ctx->optlen = 1;
>
> -        /* Userspace buffer is PAGE_SIZE * 2, but BPF
> -         * program can only see the first PAGE_SIZE
> +        /* Userspace buffer is page_size * 2, but BPF
> +         * program can only see the first page_size
>           * bytes of data.
>           */
> -        if (optval_end - optval != PAGE_SIZE)
> +        if (optval_end - optval != page_size)
>              return 0; /* EPERM, unexpected data size */
>
>          return 1;
> @@ -131,7 +129,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
>           * let next BPF program in the cgroup chain or kernel
>           * handle it.
>           */
> -        ctx->optlen = 0; /* bypass optval>PAGE_SIZE */
> +        ctx->optlen = 0; /* bypass optval>page_size */
>          return 1;
>      }
>
> @@ -160,8 +158,8 @@ int _setsockopt(struct bpf_sockopt *ctx)
>      }
>
>      if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
> -        /* Original optlen is larger than PAGE_SIZE. */
> -        if (ctx->optlen != PAGE_SIZE * 2)
> +        /* Original optlen is larger than page_size. */
> +        if (ctx->optlen != page_size * 2)
>              return 0; /* EPERM, unexpected data size */
>
>          if (optval + 1 > optval_end)
> @@ -171,11 +169,11 @@ int _setsockopt(struct bpf_sockopt *ctx)
>          optval[0] = 0;
>          ctx->optlen = 1;
>
> -        /* Usepace buffer is PAGE_SIZE * 2, but BPF
> -         * program can only see the first PAGE_SIZE
> +        /* Usepace buffer is page_size * 2, but BPF
> +         * program can only see the first page_size
>           * bytes of data.
>           */
> -        if (optval_end - optval != PAGE_SIZE)
> +        if (optval_end - optval != page_size)
>              return 0; /* EPERM, unexpected data size */
>
>          return 1;
>
>
> --
> WBR, Yauheni
>