Re: [PATCH v4 bpf-next 08/11] bpf: Special verifier handling for bpf_rbtree_{remove, first}

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Fri, Feb 10, 2023 at 04:11:25AM CET, Alexei Starovoitov wrote:
> 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, &regs[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
>

Separate type flag looks cleaner to me too, especially now that such non-owning
references have concrete semantics and context associated with them.

> 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.
>

+1

> wdyt?