Re: [PATCH bpf-next 08/24] selftests/bpf: Fix verify_pkcs7_sig on s390x

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Wed, 2023-01-25 at 17:06 -0800, Andrii Nakryiko wrote:
> On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> 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@linux.ibm.com>
> > ---
> >  .../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
> >