On Thu, Feb 09, 2023 at 09:41:41AM -0800, Dave Marchevsky wrote:
> @@ -9924,11 +9934,12 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> meta.func_id == special_kfunc_list[KF_bpf_list_pop_back]) {
> struct btf_field *field = meta.arg_list_head.field;
>
> - mark_reg_known_zero(env, regs, BPF_REG_0);
> - regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC;
> - regs[BPF_REG_0].btf = field->graph_root.btf;
> - regs[BPF_REG_0].btf_id = field->graph_root.value_btf_id;
> - regs[BPF_REG_0].off = field->graph_root.node_offset;
> + mark_reg_graph_node(regs, BPF_REG_0, &field->graph_root);
> + } else if (meta.func_id == special_kfunc_list[KF_bpf_rbtree_remove] ||
> + meta.func_id == special_kfunc_list[KF_bpf_rbtree_first]) {
> + struct btf_field *field = meta.arg_rbtree_root.field;
> +
> + mark_reg_graph_node(regs, BPF_REG_0, &field->graph_root);
> } else if (meta.func_id == special_kfunc_list[KF_bpf_cast_to_kern_ctx]) {
> mark_reg_known_zero(env, regs, BPF_REG_0);
> regs[BPF_REG_0].type = PTR_TO_BTF_ID | PTR_TRUSTED;
> @@ -9994,7 +10005,13 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> if (is_kfunc_ret_null(&meta))
> regs[BPF_REG_0].id = id;
> regs[BPF_REG_0].ref_obj_id = id;
> + } else if (meta.func_id == special_kfunc_list[KF_bpf_rbtree_first]) {
> + ref_set_non_owning_lock(env, ®s[BPF_REG_0]);
> }
Looking at the above code where R0 state is set across two different if-s
it feels that bool non_owning_ref_lock from patch 2 shouldn't be a bool.
Patch 7 also has this split initialization of the reg state.
First it does mark_reg_graph_node() which sets regs[regno].type = PTR_TO_BTF_ID | MEM_ALLOC
and then it does ref_set_non_owning_lock() that sets that bool flag.
Setting PTR_TO_BTF_ID | MEM_ALLOC in the helper without setting ref_obj_id > 0
at the same time feels error prone.
This non_owning_ref_lock bool flag is actually a just flag.
I think it would be cleaner to make it similar to MEM_ALLOC and call it
NON_OWN_REF = BIT(14 + BPF_BASE_TYPE_BITS).
Then we can set it at once in this patch and in patch 7 and avoid this split init.
The check in patch 2 also will become cleaner.
Instead of:
if (type_is_ptr_alloc_obj(reg->type) && reg->non_owning_ref_lock)
it will be
if (reg->type == PTR_TO_BTF_ID | MEM_ALLOC | NON_OWN_REF)
the transition from owning to non-owning would be easier to follow as well:
PTR_TO_BTF_ID | MEM_ALLOC with ref_obj_id > 0
->
PTR_TO_BTF_ID | MEM_ALLOC | NON_OWN_REF with ref_obj_id == 0
and it will probably help to avoid bugs where PTR_TO_BTF_ID | MEM_ALLOC is accepted
but we forgot to check ref_obj_id. There are no such places now, but it feels
less error prone with proper flag instead of bool.
I would also squash patches 1 and 2. Since we've analyzed correctness of patch 2 well enough
it doesn't make sense to go through the churn in patch 1 just to delete it in patch 2.
wdyt?
