On Wed, Mar 24, 2021 at 6:28 AM Yauheni Kaliuta
<yauheni.kaliuta@xxxxxxxxxx> wrote:
>
> Hi, Andrii!
>
> On Tue, Mar 2, 2021 at 7:08 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Mon, Mar 1, 2021 at 1:02 AM Yauheni Kaliuta
> > <yauheni.kaliuta@xxxxxxxxxx> 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
>
