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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Thu, May 19, 2022 at 7:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> +void test_unpriv_bpf_disabled(void)
> +{
> +       char *map_paths[NUM_MAPS] = {   PINPATH "array",
> +                                       PINPATH "percpu_array",
> +                                       PINPATH "hash",
> +                                       PINPATH "percpu_hash",
> +                                       PINPATH "perfbuf",
> +                                       PINPATH "ringbuf",
> +                                       PINPATH "prog_array" };
> +       int map_fds[NUM_MAPS];
> +       struct test_unpriv_bpf_disabled *skel;
> +       char unprivileged_bpf_disabled_orig[32] = {};
> +       char perf_event_paranoid_orig[32] = {};
> +       struct bpf_prog_info prog_info = {};
> +       __u32 prog_info_len = sizeof(prog_info);
> +       struct perf_event_attr attr = {};
> +       int prog_fd, perf_fd, i, ret;
> +       __u64 save_caps = 0;
> +       __u32 prog_id;
> +
> +       skel = test_unpriv_bpf_disabled__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "skel_open"))
> +               return;
> +
> +       skel->bss->test_pid = getpid();
> +
> +       map_fds[0] = bpf_map__fd(skel->maps.array);
> +       map_fds[1] = bpf_map__fd(skel->maps.percpu_array);
> +       map_fds[2] = bpf_map__fd(skel->maps.hash);
> +       map_fds[3] = bpf_map__fd(skel->maps.percpu_hash);
> +       map_fds[4] = bpf_map__fd(skel->maps.perfbuf);
> +       map_fds[5] = bpf_map__fd(skel->maps.ringbuf);
> +       map_fds[6] = bpf_map__fd(skel->maps.prog_array);
> +
> +       for (i = 0; i < NUM_MAPS; i++)
> +               ASSERT_OK(bpf_obj_pin(map_fds[i], map_paths[i]), "pin map_fd");
> +
> +       /* allow user without caps to use perf events */
> +       if (!ASSERT_OK(sysctl_set("/proc/sys/kernel/perf_event_paranoid", perf_event_paranoid_orig,
> +                                 "-1"),
> +                      "set_perf_event_paranoid"))
> +               goto cleanup;
> +       /* ensure unprivileged bpf disabled is set */
> +       ret = sysctl_set("/proc/sys/kernel/unprivileged_bpf_disabled",
> +                        unprivileged_bpf_disabled_orig, "2");
> +       if (ret == -EPERM) {
> +               /* if unprivileged_bpf_disabled=1, we get -EPERM back; that's okay. */
> +               if (!ASSERT_OK(strcmp(unprivileged_bpf_disabled_orig, "1"),
> +                              "unpriviliged_bpf_disabled_on"))
> +                       goto cleanup;
> +       } else {
> +               if (!ASSERT_OK(ret, "set unpriviliged_bpf_disabled"))
> +                       goto cleanup;
> +       }

Alan,

same as in v3 the BPF CI complained when selftests are built with clang.

/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c:267:7:
error: variable 'perf_fd' is used uninitialized whenever 'if'
condition is true [-Werror,-Wsometimes-uninitialized]
                if (!ASSERT_OK(ret, "set unpriviliged_bpf_disabled"))
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c:301:8:
note: uninitialized use occurs here
        close(perf_fd);
              ^~~~~~~
/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c:267:3:
note: remove the 'if' if its condition is always false
                if (!ASSERT_OK(ret, "set unpriviliged_bpf_disabled"))
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Looks like clang found the real issue.
I've addressed with:
-       int prog_fd, perf_fd, i, ret;
+       int prog_fd, perf_fd = -1, i, ret;

while applying.

Please pay attention to CI errors.

To repro do: make LLVM=1 -j