Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests verifying unprivileged bpf disabled behaviour

[Yonghong Song <yhs@fb.com>]

On 5/17/22 4:59 AM, Alan Maguire wrote:
> tests load/attach bpf prog with maps, perfbuf and ringbuf, pinning
> them.  Then effective caps are dropped and we verify we can
> 
> - pick up the pin
> - create ringbuf/perfbuf
> - get ringbuf/perfbuf events, carry out map update, lookup and delete
> - create a link
> 
> Negative testing also ensures
> 
> - BPF prog load fails
> - BPF map create fails
> - get fd by id fails
> - get next id fails
> - query fails
> - BTF load fails
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

LGTM except a few minor nits below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../bpf/prog_tests/unpriv_bpf_disabled.c      | 308 ++++++++++++++++++
>   .../bpf/progs/test_unpriv_bpf_disabled.c      |  83 +++++
>   2 files changed, 391 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_unpriv_bpf_disabled.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
> new file mode 100644
> index 000000000000..7c58c4f7ecc7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022, Oracle and/or its affiliates. */
> +
> +#include <test_progs.h>
> +#include <bpf/btf.h>
> +
> +#include "test_unpriv_bpf_disabled.skel.h"
> +
> +#include "cap_helpers.h"
> +
> +#define ADMIN_CAPS (1ULL << CAP_SYS_ADMIN |	\
> +		    1ULL << CAP_NET_ADMIN |	\
> +		    1ULL << CAP_PERFMON |	\
> +		    1ULL << CAP_BPF)

Not sure whether we could simply disable all capabilities
for this particular test since we know what capabilities
are need for bpf subsystem.

> +
> +#define PINPATH		"/sys/fs/bpf/unpriv_bpf_disabled_"
> +
> +struct test_unpriv_bpf_disabled *skel;
> +__u32 prog_id;
> +int prog_fd;
> +int perf_fd;

int prog_fd, perf_fd?

> +char *map_paths[7] =	{ PINPATH "array",
> +			  PINPATH "percpu_array",
> +			  PINPATH "hash",
> +			  PINPATH "percpu_hash",
> +			  PINPATH "perfbuf",
> +			  PINPATH "ringbuf",
> +			  PINPATH "prog_array" };

define a macro for '7' and used below as well?

> +int map_fds[7];
> +
> +static __u32 got_perfbuf_val;
> +static __u32 got_ringbuf_val;
> +
> +static int process_ringbuf(void *ctx, void *data, size_t len)
> +{
> +	if (len == sizeof(__u32))

ASSERT if len != sizeof(__u32)?

> +		got_ringbuf_val = *(__u32 *)data;
> +	return 0;
> +}
> +
> +static void process_perfbuf(void *ctx, int cpu, void *data, __u32 len)
> +{
> +	if (len == sizeof(__u32))

ASSERT if len != sizeof(__u32)?

> +		got_perfbuf_val = *(__u32 *)data;
> +}
> +
> +static int sysctl_set(const char *sysctl_path, char *old_val, const char *new_val)
> +{
> +	int ret = 0;
> +	FILE *fp;
> +
> +	fp = fopen(sysctl_path, "r+");
> +	if (!fp)
> +		return -errno;
> +	if (old_val && fscanf(fp, "%s", old_val) <= 0) {
> +		ret = -ENOENT;
> +	} else if (!old_val || strcmp(old_val, new_val) != 0) {
> +		fseek(fp, 0, SEEK_SET);
> +		if (fprintf(fp, "%s", new_val) < 0)
> +			ret = -errno;
> +	}
> +	fclose(fp);
> +
> +	return ret;
> +}
> +
> +static void test_unpriv_bpf_disabled_positive(void)
> +{
> +	struct perf_buffer *perfbuf = NULL;
> +	struct ring_buffer *ringbuf = NULL;
> +	int i, nr_cpus, link_fd = -1;
> +
> +	nr_cpus = bpf_num_possible_cpus();
> +
> +	skel->bss->perfbuf_val = 1;
> +	skel->bss->ringbuf_val = 2;
> +
> +	/* Positive tests for unprivileged BPF disabled. Verify we can
> +	 * - retrieve and interact with pinned maps;
> +	 * - set up and interact with perf buffer;
> +	 * - set up and interact with ring buffer;
> +	 * - create a link
> +	 */
> +	perfbuf = perf_buffer__new(bpf_map__fd(skel->maps.perfbuf), 8, process_perfbuf, NULL, NULL,
> +				   NULL);
> +	if (!ASSERT_OK_PTR(perfbuf, "perf_buffer__new"))
> +		goto cleanup;
> +
> +	ringbuf = ring_buffer__new(bpf_map__fd(skel->maps.ringbuf), process_ringbuf, NULL, NULL);
> +	if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
> +		goto cleanup;
> +
> +	/* trigger & validate perf event, ringbuf output */
> +	usleep(1);
> +
> +	ASSERT_GT(perf_buffer__poll(perfbuf, 100), -1, "perf_buffer__poll");
> +
> +	ASSERT_EQ(got_perfbuf_val, skel->bss->perfbuf_val, "check_perfbuf_val");
> +
> +	ASSERT_EQ(ring_buffer__consume(ringbuf), 1, "ring_buffer__consume");
> +
> +	ASSERT_EQ(got_ringbuf_val, skel->bss->ringbuf_val, "check_ringbuf_val");

You can remove empty lines between above ASSERT_* statements.

> +
> +	for (i = 0; i < ARRAY_SIZE(map_fds); i++) {
> +		map_fds[i] = bpf_obj_get(map_paths[i]);
> +		if (!ASSERT_GT(map_fds[i], -1, "obj_get"))
> +			goto cleanup;
> +	}
> +
[...]