On 10/25/23 2:40 PM, Dave Marchevsky wrote:
Consider this BPF program:
struct cgv_node {
int d;
struct bpf_refcount r;
struct bpf_rb_node rb;
};
struct val_stash {
struct cgv_node __kptr *v;
};
struct {
__uint(type, BPF_MAP_TYPE_ARRAY);
__type(key, int);
__type(value, struct val_stash);
__uint(max_entries, 10);
} array_map SEC(".maps");
long bpf_program(void *ctx)
{
struct val_stash *mapval;
struct cgv_node *p;
int idx = 0;
mapval = bpf_map_lookup_elem(&array_map, &idx);
if (!mapval || !mapval->v) { /* omitted */ }
p = bpf_refcount_acquire(mapval->v); /* Verification FAILs here */
/* Add p to some tree */
return 0;
}
Verification fails on the refcount_acquire:
160: (79) r1 = *(u64 *)(r8 +8) ; R1_w=untrusted_ptr_or_null_cgv_node(id=11,off=0,imm=0) R8_w=map_value(id=10,off=0,ks=8,vs=16,imm=0) refs=6
161: (b7) r2 = 0 ; R2_w=0 refs=6
162: (85) call bpf_refcount_acquire_impl#117824
arg#0 is neither owning or non-owning ref
The above verifier dump is actually from sched_ext's scx_flatcg [0],
which is the motivating usecase for this series' changes. Specifically,
scx_flatcg stashes a rb_node type w/ cgroup-specific info (struct
cgv_node) in a map when the cgroup is created, then later puts that
cgroup's node in a rbtree in .enqueue . Making struct cgv_node
refcounted would simplify the code a bit by virtue of allowing us to
remove the kptr_xchg's, but "later puts that cgroups node in a rbtree"
is not possible without a refcount_acquire, which suffers from the above
verification failure.
If we get rid of PTR_UNTRUSTED flag, and add MEM_ALLOC | NON_OWN_REF,
mapval->v would be a non-owning ref and verification would succeed. Due
to the most recent set of refcount changes [1], which modified
bpf_obj_drop behavior to not reuse refcounted graph node's underlying
memory until after RCU grace period, this is safe to do. Once mapval->v
has the correct flags it _is_ a non-owning reference and verification of
the motivating example will succeed.
[0]: https://github.com/sched-ext/sched_ext/blob/52911e1040a0f94b9c426dddcc00be5364a7ae9f/tools/sched_ext/scx_flatcg.bpf.c#L275
[1]: https://lore.kernel.org/bpf/20230821193311.3290257-1-davemarchevsky@xxxxxx/
Summary of patches:
* Patch 1 fixes an issue with bpf_refcount_acquire verification
letting MAYBE_NULL ptrs through
* Patch 2 tests Patch 1's fix
* Patch 3 broadens the use of "free only after RCU GP" to all
user-allocated types
* Patch 4 is a small nonfunctional refactoring
* Patch 5 changes verifier to mark direct LD of stashed graph node
kptr as non-owning ref
* Patch 6 tests Patch 5's verifier changes
Dave Marchevsky (6):
bpf: Add KF_RCU flag to bpf_refcount_acquire_impl
selftests/bpf: Add test passing MAYBE_NULL reg to bpf_refcount_acquire
bpf: Use bpf_mem_free_rcu when bpf_obj_dropping non-refcounted nodes
bpf: Move GRAPH_{ROOT,NODE}_MASK macros into btf_field_type enum
bpf: Mark direct ld of stashed bpf_{rb,list}_node as non-owning ref
selftests/bpf: Test bpf_refcount_acquire of node obtained via direct
ld
include/linux/bpf.h | 4 +-
kernel/bpf/btf.c | 11 ++-
kernel/bpf/helpers.c | 7 +-
kernel/bpf/verifier.c | 36 ++++++++--
.../bpf/prog_tests/local_kptr_stash.c | 33 +++++++++
.../selftests/bpf/progs/local_kptr_stash.c | 68 +++++++++++++++++++
.../bpf/progs/refcounted_kptr_fail.c | 19 ++++++
7 files changed, 159 insertions(+), 19 deletions(-)
The patch looks good to me from high level.
There is a test failure and I added some comment in Patch 5.
Please take a look and address the test failure. Thanks!
