Ensure that for dynptr constructors (MEM_UNINIT arg_type) taking a
referenced object (ref_obj_id) as their memory source, we set the
ref_obj_id of the dynptr appropriately as well. This allows us to
tie the lifetime of the dynptr to its source and properly invalidate
it when the source object is released.
For helpers, we don't have such cases yet as bpf_dynptr_from_mem is
not permitted for anything other than PTR_TO_MAP_VALUE, but still pass
meta->ref_obj_id as clone_ref_obj_id in case this is relaxed in future.
Since they are ossified we know dynptr_from_mem is the only relevant
helper and takes one memory argument, so we know the meta->ref_obj_id if
non-zero will belong to it.
For kfuncs, make sure for constructors, only 1 ref_obj_id argument is
seen, as more than one can be ambiguous in terms of ref_obj_id transfer.
Since more kfuncs can be added with possibly multiple memory arguments,
make sure meta->ref_obj_id reassignment won't cause incorrect lifetime
analysis in the future using ref_obj_cnt logic. Set this ref_obj_id as
the clone_ref_obj_id, so that it is transferred to the spilled_ptr stack
slot register state.
Add support to unmark_stack_slots_dynptr to not descend to its child
slices (using bool slice parameter) so that we don't have circular calls
when invoking release_reference. With this logic in place, we may have
the following object relationships:
+-- slice 1 (ref=1)
source (ref=1) --> dynptr (ref=1) --|-- slice 2 (ref=1)
+-- slice 3 (ref=1)
Destroying dynptr prunes the dynptr and all its children slices, but
does not affect the source. Releasing the source will effectively prune
the entire ownership tree. Dynptr clones with same ref=1 will be
parallel in the ownership tree.
+-- orig dptr (ref=1) --> slice 1 (ref=1)
source (ref=1) --|-- clone dptr (ref=1) --> slice 2 (ref=1)
+-- clone dptr (ref=1) --> slice 3 (ref=1)
In such a case, only child slices of the dynptr clone being destroyed
are invalidated. Likewise, if the source object goes away, the whole
tree ends up getting pruned.
Cc: Amery Hung <ameryhung@xxxxxxxxx>
Fixes: 81bb1c2c3e8e ("bpf: net_sched: Add basic bpf qdisc kfuncs")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
---
kernel/bpf/verifier.c | 81 ++++++++++++++++++++++++++++---------------
1 file changed, 54 insertions(+), 27 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 54c6953a8b84..a62dfab9aea6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -198,7 +198,7 @@ struct bpf_verifier_stack_elem {
static int acquire_reference(struct bpf_verifier_env *env, int insn_idx);
static int release_reference_nomark(struct bpf_verifier_state *state, int ref_obj_id);
-static int release_reference(struct bpf_verifier_env *env, int ref_obj_id);
+static int release_reference(struct bpf_verifier_env *env, int ref_obj_id, bool objects);
static void invalidate_non_owning_refs(struct bpf_verifier_env *env);
static bool in_rbtree_lock_required_cb(struct bpf_verifier_env *env);
static int ref_set_non_owning(struct bpf_verifier_env *env,
@@ -299,6 +299,7 @@ struct bpf_kfunc_call_arg_meta {
const char *func_name;
/* Out parameters */
u32 ref_obj_id;
+ u32 ref_obj_cnt;
u8 release_regno;
bool r0_rdonly;
u32 ret_btf_id;
@@ -759,7 +760,7 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
mark_dynptr_stack_regs(env, &state->stack[spi].spilled_ptr,
&state->stack[spi - 1].spilled_ptr, type);
- if (dynptr_type_refcounted(type)) {
+ if (dynptr_type_refcounted(type) || clone_ref_obj_id) {
/* The id is used to track proper releasing */
int id;
@@ -818,22 +819,19 @@ static void invalidate_dynptr(struct bpf_verifier_env *env, struct bpf_func_stat
state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
}
-static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+static int __unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_func_state *state,
+ int spi, bool slice)
{
- struct bpf_func_state *state = func(env, reg);
- int spi, ref_obj_id, i;
+ u32 ref_obj_id;
+ int i;
- spi = dynptr_get_spi(env, reg);
- if (spi < 0)
- return spi;
+ ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
- if (!dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
+ if (!dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type) && !ref_obj_id) {
invalidate_dynptr(env, state, spi);
return 0;
}
- ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
-
/* If the dynptr has a ref_obj_id, then we need to invalidate
* two things:
*
@@ -842,7 +840,8 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
*/
/* Invalidate any slices associated with this dynptr */
- WARN_ON_ONCE(release_reference(env, ref_obj_id));
+ if (slice)
+ WARN_ON_ONCE(release_reference(env, ref_obj_id, false));
/* Invalidate any dynptr clones */
for (i = 1; i < state->allocated_stack / BPF_REG_SIZE; i++) {
@@ -864,6 +863,18 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
return 0;
}
+static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg, bool slice)
+{
+ struct bpf_func_state *state = func(env, reg);
+ int spi;
+
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return spi;
+
+ return __unmark_stack_slots_dynptr(env, state, spi, slice);
+}
+
static void __mark_reg_unknown(const struct bpf_verifier_env *env,
struct bpf_reg_state *reg);
@@ -1075,7 +1086,7 @@ static int unmark_stack_slots_iter(struct bpf_verifier_env *env,
struct bpf_reg_state *st = &slot->spilled_ptr;
if (i == 0)
- WARN_ON_ONCE(release_reference(env, st->ref_obj_id));
+ WARN_ON_ONCE(release_reference(env, st->ref_obj_id, false));
__mark_reg_not_init(env, st);
@@ -9749,7 +9760,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
true, meta);
break;
case ARG_PTR_TO_DYNPTR:
- err = process_dynptr_func(env, regno, insn_idx, arg_type, 0);
+ err = process_dynptr_func(env, regno, insn_idx, arg_type, meta->ref_obj_id);
if (err)
return err;
break;
@@ -10220,12 +10231,12 @@ static int release_reference_nomark(struct bpf_verifier_state *state, int ref_ob
*
* This is the release function corresponding to acquire_reference(). Idempotent.
*/
-static int release_reference(struct bpf_verifier_env *env, int ref_obj_id)
+static int release_reference(struct bpf_verifier_env *env, int ref_obj_id, bool objects)
{
struct bpf_verifier_state *vstate = env->cur_state;
struct bpf_func_state *state;
struct bpf_reg_state *reg;
- int err;
+ int err, spi;
err = release_reference_nomark(vstate, ref_obj_id);
if (err)
@@ -10236,6 +10247,19 @@ static int release_reference(struct bpf_verifier_env *env, int ref_obj_id)
mark_reg_invalid(env, reg);
}));
+ if (!objects)
+ return 0;
+
+ bpf_for_each_spilled_reg(spi, state, reg, (1 << STACK_DYNPTR)) {
+ if (!reg)
+ continue;
+ if (!reg->dynptr.first_slot || reg->ref_obj_id != ref_obj_id)
+ continue;
+ err = __unmark_stack_slots_dynptr(env, state, spi, false);
+ if (err)
+ return err;
+ }
+
return 0;
}
@@ -11357,7 +11381,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
verbose(env, "verifier internal error: CONST_PTR_TO_DYNPTR cannot be released\n");
return -EFAULT;
}
- err = unmark_stack_slots_dynptr(env, ®s[meta.release_regno]);
+ err = unmark_stack_slots_dynptr(env, ®s[meta.release_regno], true);
} else if (func_id == BPF_FUNC_kptr_xchg && meta.ref_obj_id) {
u32 ref_obj_id = meta.ref_obj_id;
bool in_rcu = in_rcu_cs(env);
@@ -11379,7 +11403,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
}));
}
} else if (meta.ref_obj_id) {
- err = release_reference(env, meta.ref_obj_id);
+ err = release_reference(env, meta.ref_obj_id, true);
} else if (register_is_null(®s[meta.release_regno])) {
/* meta.ref_obj_id can only be 0 if register that is meant to be
* released is NULL, which must be > R0.
@@ -12974,6 +12998,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
meta->ref_obj_id = reg->ref_obj_id;
if (is_kfunc_release(meta))
meta->release_regno = regno;
+ meta->ref_obj_cnt++;
}
ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
@@ -13100,13 +13125,19 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
case KF_ARG_PTR_TO_DYNPTR:
{
enum bpf_arg_type dynptr_arg_type = ARG_PTR_TO_DYNPTR;
- int clone_ref_obj_id = 0;
+ int clone_ref_obj_id = meta->ref_obj_id;
if (reg->type == CONST_PTR_TO_DYNPTR)
dynptr_arg_type |= MEM_RDONLY;
- if (is_kfunc_arg_uninit(btf, &args[i]))
+ if (is_kfunc_arg_uninit(btf, &args[i])) {
dynptr_arg_type |= MEM_UNINIT;
+ /* It's confusing if dynptr constructor takes multiple referenced arguments. */
+ if (meta->ref_obj_cnt > 1) {
+ verbose(env, "verifier internal error: multiple referenced arguments\n");
+ return -EFAULT;
+ }
+ }
if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_skb]) {
dynptr_arg_type |= DYNPTR_TYPE_SKB;
@@ -13582,7 +13613,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
* PTR_TO_BTF_ID in bpf_kfunc_arg_meta, do the release now.
*/
if (meta.release_regno) {
- err = release_reference(env, regs[meta.release_regno].ref_obj_id);
+ err = release_reference(env, regs[meta.release_regno].ref_obj_id, true);
if (err) {
verbose(env, "kfunc %s#%d reference has not been acquired before\n",
func_name, meta.func_id);
@@ -13603,7 +13634,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
return err;
}
- err = release_reference(env, release_ref_obj_id);
+ err = release_reference(env, release_ref_obj_id, true);
if (err) {
verbose(env, "kfunc %s#%d reference has not been acquired before\n",
func_name, meta.func_id);
@@ -13803,11 +13834,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
return -EFAULT;
}
regs[BPF_REG_0].dynptr_id = meta.initialized_dynptr.id;
-
- /* we don't need to set BPF_REG_0's ref obj id
- * because packet slices are not refcounted (see
- * dynptr_type_refcounted)
- */
+ regs[BPF_REG_0].ref_obj_id = meta.initialized_dynptr.ref_obj_id;
} else {
verbose(env, "kernel function %s unhandled dynamic return type\n",
meta.func_name);
--
2.47.1
