Received-SPF: pass (google.com: domain of linux-kernel+bounces-15936-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240103153307.553838-1-brho@google.com> <20240103153307.553838-3-brho@google.com>
In-Reply-To: <20240103153307.553838-3-brho@google.com>
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date: Wed, 3 Jan 2024 11:51:13 -0800
Message-ID: <CAEf4BzbKT3LbHQSFwpAfoJuhyGy2NpHk7A6ivkFiutN_jnKHYg@mail.gmail.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: add inline assembly helpers
 to access array elements
To: Barret Rhoden <brho@google.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, Alexei Starovoitov <ast@kernel.org>, 
	Daniel Borkmann <daniel@iogearbox.net>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, mattbobrowski@google.com, bpf@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 3, 2024 at 7:33=E2=80=AFAM Barret Rhoden <brho@google.com> wrot=
e:
>
> When accessing an array, even if you insert your own bounds check,
> sometimes the compiler will remove the check, or modify it such that the
> verifier no longer knows your access is within bounds.
>
> The compiler is even free to make a copy of a register, check the copy,
> and use the original to access the array.  The verifier knows the *copy*
> is within bounds, but not the original register!
>
> Signed-off-by: Barret Rhoden <brho@google.com>
> ---
>  tools/testing/selftests/bpf/Makefile          |   2 +-
>  .../bpf/prog_tests/test_array_elem.c          | 112 ++++++++++
>  .../selftests/bpf/progs/array_elem_test.c     | 195 ++++++++++++++++++
>  tools/testing/selftests/bpf/progs/bpf_misc.h  |  43 ++++
>  4 files changed, 351 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_array_ele=
m.c
>  create mode 100644 tools/testing/selftests/bpf/progs/array_elem_test.c
>

I'm curious how bpf_cmp_likely/bpf_cmp_unlikely (just applied to
bpf-next) compares to this?


> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftes=
ts/bpf/Makefile
> index 617ae55c3bb5..651d4663cc78 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -34,7 +34,7 @@ LIBELF_CFLAGS :=3D $(shell $(PKG_CONFIG) libelf --cflag=
s 2>/dev/null)
>  LIBELF_LIBS    :=3D $(shell $(PKG_CONFIG) libelf --libs 2>/dev/null || e=
cho -lelf)
>
>  CFLAGS +=3D -g $(OPT_FLAGS) -rdynamic                                   =
 \
> -         -Wall -Werror                                                 \
> +         -dicks -Wall -Werror                                          \

what does this magic argument do? )

>           $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)                    \
>           -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)          \
>           -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_array_elem.c b/t=
ools/testing/selftests/bpf/prog_tests/test_array_elem.c
> new file mode 100644
> index 000000000000..c953636f07c9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_array_elem.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Google LLC. */
> +#include <test_progs.h>
> +#include "array_elem_test.skel.h"
> +
> +#define NR_MAP_ELEMS 100
> +
> +/*
> + * Helper to load and run a program.
> + * Call must define skel, map_elems, and bss_elems.
> + * Destroy the skel when you're done.
> + */
> +#define load_and_run(PROG) ({

does this have to be a macro? Can you write it as a function?
                                    \
> +       int err;                                                        \
> +       skel =3D array_elem_test__open();                                =
 \
> +       if (!ASSERT_OK_PTR(skel, "array_elem_test open"))               \
> +               return;                                                 \
> +       bpf_program__set_autoload(skel->progs.x_ ## PROG, true);        \
> +       err =3D array_elem_test__load(skel);                             =
 \
> +       if (!ASSERT_EQ(err, 0, "array_elem_test load")) {               \
> +               array_elem_test__destroy(skel);                         \
> +               return;                                                 \
> +       }                                                               \
> +       err =3D array_elem_test__attach(skel);                           =
 \
> +       if (!ASSERT_EQ(err, 0, "array_elem_test attach")) {             \
> +               array_elem_test__destroy(skel);                         \
> +               return;                                                 \
> +       }                                                               \
> +       for (int i =3D 0; i < NR_MAP_ELEMS; i++)                         =
 \
> +               skel->bss->lookup_indexes[i] =3D i;                      =
 \
> +       map_elems =3D bpf_map__mmap(skel->maps.arraymap);                =
 \
> +       ASSERT_OK_PTR(map_elems, "mmap");                               \
> +       bss_elems =3D skel->bss->bss_elems;                              =
 \
> +       skel->bss->target_pid =3D getpid();                              =
 \
> +       usleep(1);                                                      \
> +})
> +


[...]