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@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. 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:
signature.asc
Description: PGP signature
