Re: [PATCH bpf-next v1 2/4] bpf: Prepare prog_test_struct kfuncs for runtime tests

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Tue, May 10, 2022 at 11:01 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Wed, May 11, 2022 at 10:07:35AM IST, Alexei Starovoitov wrote:
> > On Tue, May 10, 2022 at 2:17 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > In an effort to actually test the refcounting logic at runtime, add a
> > > refcount_t member to prog_test_ref_kfunc and use it in selftests to
> > > verify and test the whole logic more exhaustively.
> > >
> > > To ensure reading the count to verify it remains stable, make
> > > prog_test_ref_kfunc a per-CPU variable, so that inside a BPF program the
> > > count can be read reliably based on number of acquisitions made. Then,
> > > pairing them with releases and reading from the global per-CPU variable
> > > will allow verifying whether release operations put the refcount.
> >
> > The patches look good, but the per-cpu part is a puzzle.
> > The test is not parallel. Everything looks sequential
> > and there are no races.
> > It seems to me if it was
> > static struct prog_test_ref_kfunc prog_test_struct = {..};
> > and none of [bpf_]this_cpu_ptr()
> > the test would work the same way.
> > What am I missing?
>
> You are not missing anything. It would work the same. I just made it per-CPU for
> the off chance that someone runs ./test_progs -t map_kptr in parallel on the
> same machine. Then one or both might fail, since count won't just be inc/dec by
> us, and reading it would produce something other than what we expect.

I see. You should have mentioned that in the commit log.
But how per-cpu helps in this case?
prog_run is executed with cpu=0, so both test_progs -t map_kptr
will collide on the same cpu.
At the end it's the same. one or both might fail?

In general all serial_ tests in test_progs will fail in
parallel run.
Even non-serial tests might fail.
The non-serial tests are ok for test_progs -j.
They're parallel between themselves, but there are no guarantees
that every individual test can be run parallel with itself.
Majority will probably be fine, but not all.

> One other benefit is getting non-ref PTR_TO_BTF_ID to prog_test_struct to
> inspect cnt after releasing acquired pointer (using bpf_this_cpu_ptr), but that
> can also be done by non-ref kfunc returning a pointer to it.

Not following. non-ref == ptr_untrusted. That doesn't preclude
bpf prog from reading refcnt directly, but disallows passing
into helpers.
So with non-percpu the following hack
 bpf_kfunc_call_test_release(p);
 if (p_cpu->cnt.refs.counter ...)
wouldn't be necessary.
The prog could release(p) and read p->cnt.refs.counter right after.
While with per-cpu approach you had to do
p_cpu = bpf_this_cpu_ptr(&prog_test_struct);
hack and rely on intimate knowledge of the kernel side.

> If you feel it's not worth it, I will drop it in the next version.

So far I see the downsides.
Also check the CI. test_progs-no_alu32 fails:
test_map_kptr_fail_prog:PASS:map_kptr__load must fail 0 nsec
test_map_kptr_fail_prog:FAIL:expected error message unexpected error: -22
Expected: Unreleased reference id=5 alloc_insn=18
Verifier: 0: R1=ctx(off=0,imm=0) R10=fp0