On Fri, Jan 27, 2023 at 4:36 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> 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.
sgtm
>
> > > +
> > > 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.
Yep, replied there as well: BPF_PROBE_READ still using
bpf_probe_read() is an omission and we should fix that.
>
> > > if (data_val->data_len > sizeof(data_val->data))
> > > return -EINVAL;
> > > --
> > > 2.39.1
> > >
>
