| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 09 Apr 2024 02:00:57 +0300
From: Eduard Zingerman <eddyz87@...il.com>
To: Benjamin Tissoires <bentiss@...nel.org>, Alexei Starovoitov
<ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko
<andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu
<song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>, John Fastabend
<john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Stanislav
Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa
<jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>, Shuah Khan
<shuah@...nel.org>
Cc: bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH RFC bpf-next v6 6/6] selftests/bpf: add sleepable timer
tests
On Mon, 2024-04-08 at 10:09 +0200, Benjamin Tissoires wrote:
> bpf_experimental.h and ../bpf_testmod/bpf_testmod_kfunc.h are both
> including vmlinux.h, which is not compatible with including time.h
> or bpf_tcp_helpers.h.
>
> So keep sleepable tests in a separate bpf source file.
>
> The first correct test is run twice for convenience:
> - first through RUN_TESTS
> - then we ensure that the timer was actually executed, in a manual
> load/attach/run
>
> Signed-off-by: Benjamin Tissoires <bentiss@...nel.org>
>
> ---
Acked-by: Eduard Zingerman <eddyz87@...il.com>
(With a few nitpicks)
> diff --git a/tools/testing/selftests/bpf/prog_tests/timer.c b/tools/testing/selftests/bpf/prog_tests/timer.c
> index d66687f1ee6a..c6c7c623b31c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/timer.c
> +++ b/tools/testing/selftests/bpf/prog_tests/timer.c
[...]
> +void serial_test_sleepable_timer(void)
> +{
> + struct timer_sleepable *timer_sleepable_skel = NULL;
> + int err, prog_fd;
> +
> + LIBBPF_OPTS(bpf_test_run_opts, topts);
> +
> + RUN_TESTS(timer_sleepable);
> +
> + /* re-run the success test to check if the timer was actually executed */
> +
> + timer_sleepable_skel = timer_sleepable__open_and_load();
> + if (!ASSERT_OK_PTR(timer_sleepable_skel, "timer_sleepable_skel_load"))
> + return;
> +
> + err = timer_sleepable__attach(timer_sleepable_skel);
> + if (!ASSERT_OK(err, "timer_sleepable_attach"))
> + return;
Nit: this should call timer_sleepable__destroy();
> +
> + prog_fd = bpf_program__fd(timer_sleepable_skel->progs.test_syscall_sleepable);
> + err = bpf_prog_test_run_opts(prog_fd, &topts);
> + ASSERT_OK(err, "test_run");
> + ASSERT_EQ(topts.retval, 0, "test_run");
> +
> + usleep(50); /* 10 usecs should be enough, but give it extra */
> +
> + ASSERT_EQ(timer_sleepable_skel->bss->ok_sleepable, 1, "ok_sleepable");
Nit: same as above.
> +}
> diff --git a/tools/testing/selftests/bpf/progs/timer_sleepable.c b/tools/testing/selftests/bpf/progs/timer_sleepable.c
> new file mode 100644
> index 000000000000..fc7829d8b6c4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/timer_sleepable.c
[...]
> +SEC("tc")
> +/* check that calling bpf_timer_start() with BPF_F_TIMER_SLEEPABLE on a sleepable
> + * callback works
> + */
> +__retval(0)
> +long test_call_sleepable(void *ctx)
> +{
> + int key = 0;
> + struct bpf_timer *timer;
> +
> + if (ok_sleepable & 1)
> + return -1;
> +
> + timer = bpf_map_lookup_elem(&timer_map, &key);
> + if (timer) {
> + if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SLEEPABLE) != 0)
> + return -2;
> + bpf_timer_set_sleepable_cb(timer, timer_cb_sleepable);
> + if (bpf_timer_start(timer, 0, 0))
> + return -3;
> + } else {
> + return -4;
> + }
> +
> + return 0;
> +}
> +
> +SEC("syscall")
> +/* check that calling bpf_timer_start() with BPF_F_TIMER_SLEEPABLE on a sleepable
> + * callback works.
> + */
> +__retval(0)
> +long test_syscall_sleepable(void *ctx)
> +{
Nit: the body of this function is the same as in test_call_sleepable(),
maybe factor it out as an auxiliary static function?
(also, should these tests use different 'key' ?)
> + int key = 0;
> + struct bpf_timer *timer;
> +
> + if (ok_sleepable & 1)
> + return -1;
> +
> + timer = bpf_map_lookup_elem(&timer_map, &key);
> + if (timer) {
> + if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SLEEPABLE) != 0)
> + return -2;
> + bpf_timer_set_sleepable_cb(timer, timer_cb_sleepable);
> + if (bpf_timer_start(timer, 0, 0))
> + return -3;
> + } else {
> + return -4;
> + }
> +
> + return 0;
> +}
[...]
> +SEC("tc")
> +/* check that calling bpf_timer_start() with a delay on a sleepable
> + * callback is returning -EINVAL
> + */
> +__retval(-22)
> +long test_call_sleepable_delay(void *ctx)
> +{
> + int key = 2;
> + struct bpf_timer *timer;
> +
> + timer = bpf_map_lookup_elem(&timer_map, &key);
> + if (!timer)
> + return 1;
> +
> + if (bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SLEEPABLE))
> + return 2;
> +
> + if (bpf_timer_set_sleepable_cb(timer, timer_cb_sleepable))
> + return 3;
> +
> + return bpf_timer_start(timer, 1, 0);
Q: should verifier statically check that 3rd parameter is zero for sleepable timers?
(same question for call to bpf_timer_set_sleepable_cb() with non-sleepable map)
[...]
Powered by blists - more mailing lists