On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> Use bpf_probe_read_kernel() instead of bpf_probe_read(), which is not
> defined on all architectures.
>
> While at it, improve the error handling: do not hide the verifier log,
> and check the return values of bpf_probe_read_kernel() and
> bpf_copy_from_user().
>
> Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> ---
> .../selftests/bpf/prog_tests/verify_pkcs7_sig.c | 9 +++++++++
> .../selftests/bpf/progs/test_verify_pkcs7_sig.c | 12 ++++++++----
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> index 579d6ee83ce0..75c256f79f85 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> @@ -56,11 +56,17 @@ struct data {
> __u32 sig_len;
> };
>
> +static char libbpf_log[8192];
> static bool kfunc_not_supported;
>
> static int libbpf_print_cb(enum libbpf_print_level level, const char *fmt,
> va_list args)
> {
> + size_t log_len = strlen(libbpf_log);
> +
> + vsnprintf(libbpf_log + log_len, sizeof(libbpf_log) - log_len,
> + fmt, args);
it seems like test is written to assume that load might fail and we'll
get error messages, so not sure it's that useful to print out these
errors. But at the very least we should filter out DEBUG and INFO
level messages, and pass through WARN only.
Also, there is no point in having a separate log buffer, just printf
directly. test_progs will take care to collect overall log and ignore
it if test succeeds, or emit it if test fails
> +
> if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found in kernel or module BTFs\n"))
> return 0;
>
> @@ -277,6 +283,7 @@ void test_verify_pkcs7_sig(void)
> if (!ASSERT_OK_PTR(skel, "test_verify_pkcs7_sig__open"))
> goto close_prog;
>
> + libbpf_log[0] = 0;
> old_print_cb = libbpf_set_print(libbpf_print_cb);
> ret = test_verify_pkcs7_sig__load(skel);
> libbpf_set_print(old_print_cb);
> @@ -289,6 +296,8 @@ void test_verify_pkcs7_sig(void)
> goto close_prog;
> }
>
> + printf("%s", libbpf_log);
> +
> if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__load"))
> goto close_prog;
>
> diff --git a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> index ce419304ff1f..7748cc23de8a 100644
> --- a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> +++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> @@ -59,10 +59,14 @@ int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size)
> if (!data_val)
> return 0;
>
> - bpf_probe_read(&value, sizeof(value), &attr->value);
> -
> - bpf_copy_from_user(data_val, sizeof(struct data),
> - (void *)(unsigned long)value);
> + ret = bpf_probe_read_kernel(&value, sizeof(value), &attr->value);
> + if (ret)
> + return ret;
> +
> + ret = bpf_copy_from_user(data_val, sizeof(struct data),
> + (void *)(unsigned long)value);
> + if (ret)
> + return ret;
this part looks good, we shouldn't use bpf_probe_read.
You'll have to update progs/profiler.inc.h as well, btw, which still
uses bpf_probe_read() and bpf_probe_read_str.
>
> if (data_val->data_len > sizeof(data_val->data))
> return -EINVAL;
> --
> 2.39.1
>
