Re: [PATCH bpf-next v1 09/29] bpf: Register cleanup destructors for more types

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: btf_find_dtor() misses nf_conn destructors when
  CONFIG_NF_CONNTRACK=m
--

commit ddb65cf317bc73659c05f7b86c4aa44d5480e3c1
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Register cleanup destructors for more types

This commit registers BPF_DTOR_CLEANUP destructors and void (*)(void *)
wrappers for open-coded iterators and referenced types across the kernel
so that stack unwinding on bpf_throw() can release held references.

> diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> index c2df7c9482812..9d9d4e97ca4e5 100644
> --- a/net/netfilter/nf_conntrack_bpf.c
> +++ b/net/netfilter/nf_conntrack_bpf.c
[ ... ]
> @@ -537,12 +553,34 @@ static const struct btf_kfunc_id_set nf_conntrack_kfunc_set = {
>  	.set   = &nf_ct_kfunc_set,
>  };
>  
> +BTF_ID_LIST(nf_ct_dtor_ids)
> +BTF_ID(func, bpf_ct_release_dtor)
> +BTF_ID(func, bpf_ct_release)
> +BTF_ID(func, bpf_ct_free_dtor)
> +BTF_ID(func, bpf_ct_insert_entry)
> +
>  int register_nf_conntrack_bpf(void)
>  {
> +	/* The only kfunc that takes an allocated entry off a program is bpf_ct_insert_entry() */
> +	const struct btf_id_dtor_kfunc nf_ct_dtors[] = {
> +		{
> +			.btf_id         = btf_nf_conn_ids[0],
> +			.kfunc_btf_id   = nf_ct_dtor_ids[0],
> +			.release_btf_id = nf_ct_dtor_ids[1],
> +			.flags          = BPF_DTOR_CLEANUP,
> +		},
> +		{
> +			.btf_id         = btf_nf_conn_ids[1],
> +			.kfunc_btf_id   = nf_ct_dtor_ids[2],
> +			.release_btf_id = nf_ct_dtor_ids[3],
> +			.flags          = BPF_DTOR_CLEANUP,
> +		},
> +	};
>  	int ret;
>  
>  	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &nf_conntrack_kfunc_set);
>  	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &nf_conntrack_kfunc_set);
> +	ret = ret ?: register_btf_id_dtor_kfuncs(nf_ct_dtors, ARRAY_SIZE(nf_ct_dtors), THIS_MODULE);

[Severity: Medium]
Can btf_find_dtor() find nf_ct_dtors when CONFIG_NF_CONNTRACK=m?

tc_cls_act_func_proto() in net/core/filter.c executes
BTF_TYPE_EMIT(struct nf_conn___init), so both struct nf_conn
(btf_nf_conn_ids[0]) and struct nf_conn___init (btf_nf_conn_ids[1]) are
emitted into vmlinux base BTF and have BTF IDs below nf_conntrack's module
btf->start_id.  However, register_btf_id_dtor_kfuncs() stores nf_ct_dtors in
nf_conntrack's module dtor_kfunc_tab via THIS_MODULE.

When resource_registry() looks up the destructor, btf_find_dtor() walks to
base_btf before searching the module's table:

kernel/bpf/btf.c:btf_find_dtor() {
	...
	while (btf_id < btf->start_id)
		btf = btf->base_btf;
	dtor = btf_dtor_tab_find(btf, btf_id);
	if (dtor || !btf_is_module(btf)) {
		*btfp = btf;
		return dtor;
	}
	...
}

Because btf_id < btf->start_id, btf_find_dtor() skips nf_conntrack's
dtor_kfunc_tab, searches vmlinux's table where nf_ct_dtors was not
registered, and returns NULL.  As a result, the verifier rejects any XDP or
sched_cls program that calls bpf_throw() while holding a conntrack reference
when CONFIG_NF_CONNTRACK=m.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794399387.git.memxor@gmail.com?part=9