Re: [PATCH bpf-next v7 20/26] bpf: Introduce single ownership BPF linked list API

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

On Tue, Nov 15, 2022 at 11:56:16PM IST, Alexei Starovoitov wrote:
> On Tue, Nov 15, 2022 at 10:29:51PM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Tue, Nov 15, 2022 at 11:56:37AM IST, Alexei Starovoitov wrote:
> > > On Tue, Nov 15, 2022 at 12:45:41AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > +}
> > > > +
> > > > +static int process_kf_arg_ptr_to_list_node(struct bpf_verifier_env *env,
> > > > +					   struct bpf_reg_state *reg, u32 regno,
> > > > +					   struct bpf_kfunc_call_arg_meta *meta)
> > > > +{
> > > > +	struct btf_struct_meta *struct_meta;
> > > > +	struct btf_field *field;
> > > > +	struct btf_record *rec;
> > > > +	u32 list_node_off;
> > > > +
> > > > +	if (meta->btf != btf_vmlinux ||
> > > > +	    (meta->func_id != special_kfunc_list[KF_bpf_list_push_front] &&
> > > > +	     meta->func_id != special_kfunc_list[KF_bpf_list_push_back])) {
> > > > +		verbose(env, "verifier internal error: bpf_list_head argument for unknown kfunc\n");
> > >
> > > typo. bpf_list_node ?
> > >
> > > > +		return -EFAULT;
> > > > +	}
> > > > +
> > > > +	if (!tnum_is_const(reg->var_off)) {
> > > > +		verbose(env,
> > > > +			"R%d doesn't have constant offset. bpf_list_head has to be at the constant offset\n",
> > >
> > > same typo?
> > >
> >
> > These two are typos.
> >
> > > > +			regno);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	struct_meta = btf_find_struct_meta(reg->btf, reg->btf_id);
> > > > +	if (!struct_meta) {
> > > > +		verbose(env, "bpf_list_node not found for allocated object\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +	rec = struct_meta->record;
> > > > +
> > > > +	list_node_off = reg->off + reg->var_off.value;
> > > > +	field = btf_record_find(rec, list_node_off, BPF_LIST_NODE);
> > > > +	if (!field || field->offset != list_node_off) {
> > > > +		verbose(env, "bpf_list_node not found at offset=%u\n", list_node_off);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	field = meta->arg_list_head.field;
> > > > +
> > > > +	if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, 0, field->list_head.btf,
> > > > +				  field->list_head.value_btf_id, true)) {
> > > > +		verbose(env, "bpf_list_head value type does not match arg#1\n");
> > >
> > > and the same typo again?!
> > >
> >
> > This is probably just poorly worded.
> > The value type (__contains) of bpf_list_head does not match arg#1 (node).
> >
> > What's better, maybe:
> > bpf_list_node type does not match bpf_list_head value type?
>
> That would be the case when user is trying to bpf_list_push_head
> to head that has __contains tag that point to a different node ?

Right.

> It feels the users will be hitting this error case from time to time,
> so the most verbose message is the best.
> Both options above are a bit cryptic.

How about something like this?

operation on bpf_list_head expects node at offset=X in struct foo, but
node is at offset=Y in struct bar