linux-kernel - Re: [PATCH v3 4/4] selftests/bpf: Add selftests validating the user ringbuf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 Aug 2022 08:46:12 -0500
From:   David Vernet <void@...ifault.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, andrii@...nel.org,
        daniel@...earbox.net, kernel-team@...com, martin.lau@...ux.dev,
        song@...nel.org, yhs@...com, john.fastabend@...il.com,
        kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
        jolsa@...nel.org, joannelkoong@...il.com, tj@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/4] selftests/bpf: Add selftests validating the user
 ringbuf

On Wed, Aug 24, 2022 at 03:03:54PM -0700, Andrii Nakryiko wrote:

[...]

> > +                       CHECK(read <= 0, "snprintf_comm",
> > +                             "Failed to write index %d to comm\n", i);
> 
> please, no CHECK() use in new tests, we have ASSERT_xxx() covering all
> common cases

No problem, I'll make this change in v4. Unless I missed something,
CHECK() isn't documented as deprecated in test_progs.h. I'll take care
of adding that in a separate change.

> > +static long
> > +bad_access1(struct bpf_dynptr *dynptr, void *context)
> > +{
> > +       const struct sample *sample;
> > +
> > +       sample = bpf_dynptr_data(dynptr - 1, 0, sizeof(*sample));
> > +       bpf_printk("Was able to pass bad pointer %lx\n", (__u64)dynptr - 1);
> > +
> > +       return 0;
> > +}
> > +
> > +/* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should
> > + * not be able to read before the pointer.
> > + */
> > +SEC("?raw_tp/sys_nanosleep")
> 
> there is no sys_nanosleep raw tracepoint, use SEC("?raw_tp") to
> specify type, that's enough

Got it, thanks for catching that. Will fix.

> > diff --git a/tools/testing/selftests/bpf/test_user_ringbuf.h b/tools/testing/selftests/bpf/test_user_ringbuf.h
> > new file mode 100644
> > index 000000000000..1643b4d59ba7
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_user_ringbuf.h
> 
> nit: I'd probably put it under progs/test_user_ringbuf.h so it's
> closer to BPF source code. As it is right now, it's neither near
> user-space part of tests nor near BPF part.

Fair enough, will fix.

Thanks,
David