Re: [PATCH v3 4/4] selftests/bpf: Add selftests validating the user ringbuf

Andrii Nakryiko
2022-08-24T22:04:26Z

On Thu, Aug 18, 2022 at 3:12 PM David Vernet <void@manifault.com> wrote:
>
> This change includes selftests that validate the expected behavior and
> APIs of the new BPF_MAP_TYPE_USER_RINGBUF map type.
>
> Signed-off-by: David Vernet <void@manifault.com>
> ---
> .../selftests/bpf/prog_tests/user_ringbuf.c | 755 ++++++++++++++++++
> .../selftests/bpf/progs/user_ringbuf_fail.c | 177 ++++
> .../bpf/progs/user_ringbuf_success.c | 220 +++++
> .../testing/selftests/bpf/test_user_ringbuf.h | 35 +
> 4 files changed, 1187 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> create mode 100644 tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> create mode 100644 tools/testing/selftests/bpf/progs/user_ringbuf_success.c
> create mode 100644 tools/testing/selftests/bpf/test_user_ringbuf.h
>

[...]

> + /* Write some number of samples to the ring buffer. */
> + for (i = 0; i < num_samples; i++) {
> + struct sample *entry;
> + int read;
> +
> + entry = user_ring_buffer__reserve(ringbuf, sizeof(*entry));
> + if (!entry) {
> + err = -errno;
> + goto done;
> + }
> +
> + entry->pid = getpid();
> + entry->seq = i;
> + entry->value = i * i;
> +
> + read = snprintf(entry->comm, sizeof(entry->comm), "%u", i);
> + if (read <= 0) {
> + /* Only invoke CHECK on the error path to avoid spamming
> + * logs with mostly success messages.
> + */
> + CHECK(read <= 0, "snprintf_comm",
> + "Failed to write index %d to comm\n", i);

please, no CHECK() use in new tests, we have ASSERT_xxx() covering all
common cases

> + err = read;
> + user_ring_buffer__discard(ringbuf, entry);
> + goto done;
> + }
> +
> + user_ring_buffer__submit(ringbuf, entry);
> + }
> +

[...]

> +static long
> +bad_access1(struct bpf_dynptr *dynptr, void *context)
> +{
> + const struct sample *sample;
> +
> + sample = bpf_dynptr_data(dynptr - 1, 0, sizeof(*sample));
> + bpf_printk("Was able to pass bad pointer %lx\n", (__u64)dynptr - 1);
> +
> + return 0;
> +}
> +
> +/* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should
> + * not be able to read before the pointer.
> + */
> +SEC("?raw_tp/sys_nanosleep")

there is no sys_nanosleep raw tracepoint, use SEC("?raw_tp") to
specify type, that's enough

> +int user_ringbuf_callback_bad_access1(void *ctx)
> +{
> + bpf_user_ringbuf_drain(&user_ringbuf, bad_access1, NULL, 0);
> +
> + return 0;
> +}
> +

[...]

> diff --git a/tools/testing/selftests/bpf/test_user_ringbuf.h b/tools/testing/selftests/bpf/test_user_ringbuf.h
> new file mode 100644
> index 000000000000..1643b4d59ba7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_user_ringbuf.h

nit: I'd probably put it under progs/test_user_ringbuf.h so it's
closer to BPF source code. As it is right now, it's neither near
user-space part of tests nor near BPF part.

> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +
> +#ifndef _TEST_USER_RINGBUF_H
> +#define _TEST_USER_RINGBUF_H
> +
> +#define TEST_OP_64 4
> +#define TEST_OP_32 2
> +
> +enum test_msg_op {
> + TEST_MSG_OP_INC64,
> + TEST_MSG_OP_INC32,
> + TEST_MSG_OP_MUL64,
> + TEST_MSG_OP_MUL32,
> +
> + // Must come last.
> + TEST_MSG_OP_NUM_OPS,
> +};
> +
> +struct test_msg {
> + enum test_msg_op msg_op;
> + union {
> + __s64 operand_64;
> + __s32 operand_32;
> + };
> +};
> +
> +struct sample {
> + int pid;
> + int seq;
> + long value;
> + char comm[16];
> +};
> +
> +#endif /* _TEST_USER_RINGBUF_H */
> --
> 2.37.1
>