On Wed, 2023-01-25 at 17:06 -0800, Andrii Nakryiko wrote:
> 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
Thanks, I completely overlooked the fact that the test framework
already hides the output in case of success. With that in mind I can do
just this:
--- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
+++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
@@ -61,6 +61,9 @@ static bool kfunc_not_supported;
static int libbpf_print_cb(enum libbpf_print_level level, const char
*fmt,
va_list args)
{
+ if (level == LIBBPF_WARN)
+ vprintf(fmt, args);
+
if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found in
kernel or module BTFs\n"))
return 0;
If the load fails due to missing kfuncs, we'll skip the test - I think
in this case the output won't be printed either, so we should be fine.
> > +
> > 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.
I remember trying this, but there were still failures due to, as I
thought back then, usage of BPF_CORE_READ() and the lack of
BPF_CORE_READ_KERNEL(). But this seems to be a generic issue. Let me
try again and post my findings as a reply to 0/24.
> > if (data_val->data_len > sizeof(data_val->data))
> > return -EINVAL;
> > --
> > 2.39.1
> >
