Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220818221212.464487-1-void@manifault.com> <20220818221212.464487-5-void@manifault.com>
In-Reply-To: <20220818221212.464487-5-void@manifault.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 24 Aug 2022 15:03:54 -0700
Message-ID: <CAEf4Bzbj0ACUmZqQLhRR5DvEX9Zphqz5UwBWdkTdXfKqxWM0mQ@mail.gmail.com>
Subject: Re: [PATCH v3 4/4] selftests/bpf: Add selftests validating the user ringbuf
To:     David Vernet <void@manifault.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, andrii@kernel.org,
        daniel@iogearbox.net, kernel-team@fb.com, martin.lau@linux.dev,
        song@kernel.org, yhs@fb.com, john.fastabend@gmail.com,
        kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
        jolsa@kernel.org, joannelkoong@gmail.com, tj@kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>