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

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <martin.lau@kernel.org>,
	Dave Marchevsky <davemarchevsky@meta.com>
Subject: Re: [PATCH bpf-next v7 20/26] bpf: Introduce single ownership BPF linked list API
Date: Tue, 15 Nov 2022 22:29:51 +0530	[thread overview]
Message-ID: <20221115165951.fy7bqwcum3veiz2d@apollo> (raw)
In-Reply-To: <20221115062637.hzuo7ehffpuxflsw@macbook-pro-5.dhcp.thefacebook.com>

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?