On Fri, Feb 9, 2024 at 3:14 PM David Vernet <void@xxxxxxxxxxxxx> 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. > 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. 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. > > > + 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.
