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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux