Re: [PATCH v2 bpf-next 17/20] selftests/bpf: Add unit tests for bpf_arena_alloc/free_pages

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 3783 bytes --]

On Fri, Feb 09, 2024 at 08:35:01PM -0800, Alexei Starovoitov wrote:
> On Fri, Feb 9, 2024 at 3:14 PM David Vernet <void@manifault.com> wrote:
> >
> > > +
> > > +#ifndef arena_container_of
> >
> > Why is this ifndef required if we have a pragma once above?
> 
> Just a habit to check for a macro before defining it.
> 
> > Obviously it's way better for us to actually have arenas in the interim
> > so this is fine for now, but UAF bugs could potentially be pretty
> > painful until we get proper exception unwinding support.
> 
> Detection that arena access faulted doesn't have to come after
> exception unwinding. Exceptions vs cancellable progs are also different.
> A record of the line in bpf prog that caused the first fault is probably
> good enough for prog debugging.
> 
> > Otherwise, in terms of usability, this looks really good. The only thing
> > to bear in mind is that I don't think we can fully get away from kptrs
> > that will have some duplicated logic compared to what we can enable in
> > an arena. For example, we will have to retain at least some of the
> > struct cpumask * kptrs for e.g. copying a struct task_struct's struct
> > cpumask *cpus_ptr field.
> 
> I think that's a bit orthogonal.
> task->cpus_ptr is a trusted_ptr_to_btf_id access that can be mixed
> within a program with arena access.

I see, so the idea is that we'd just use regular accesses to query the
bits in that cpumask rather than kfuncs? Similar to how we e.g. read a
task field such as p->comm with a regular load? Ok, that should work.

> > For now, we could iterate over the cpumask and manually set the bits, so
> > maybe even just supporting bpf_cpumask_test_cpu() would be enough
> > (though donig a bitmap_copy() would be better of course)? This is
> > probably fine for most use cases as we'd likely only be doing struct
> > cpumask * -> arena copies on slowpaths. But is there any kind of more
> > generalized integration we want to have between arenas and kptrs?  Not
> > sure, can't think of any off the top of my head.
> 
> Hopefully we'll be able to invent a way to store kptr-s inside the arena,
> but from a cpumask perspective bpf_cpumask_test_cpu() can be made
> polymorphic to work with arena ptrs and kptrs.
> Same with bpf_cpumask_and(). Mixed arguments can be allowed.
> Args can be either kptr or ptr_to_arena.

This would be ideal. And to make sure I understand, many of these
wouldn't even be kfuncs, right? We'd just be doing loads on two
safe/trusted objects that were both pointers to a bitmap of size
NR_CPUS?

> I still believe that we can deprecate 'struct bpf_cpumask'.
> The cpumask_t will stay, of course, but we won't need to
> bpf_obj_new(bpf_cpumask) and carefully track refcnt.
> The arena can do the same much faster.

Yes, I agree. Any use of struct bpf_cpumask * can just be stored in an
arena, and any kfuncs where we were previously passing a struct
bpf_cpumask * could instead just take an arena cpumask, or be done
entirely using BPF instructions per your point above.

> > > +             return 7;
> > > +     page3 = bpf_arena_alloc_pages(&arena, NULL, 1, NUMA_NO_NODE, 0);
> > > +     if (!page3)
> > > +             return 8;
> > > +     *page3 = 3;
> > > +     if (page2 != page3)
> > > +             return 9;
> > > +     if (*page1 != 1)
> > > +             return 10;
> >
> > Should we also test doing a store after an arena has been freed?
> 
> You mean the whole bpf arena map was freed ?
> I don't see how the verifier would allow that.
> If you meant a few pages were freed from the arena then such a test is
> already in the patches.

I meant a negative test that verifies we fail to load a prog that does a
write to a freed arena pointer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]