Re: [PATCH bpf-next v1 08/29] bpf: Add flags and release kfuncs to the dtor registry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] bpf: global state corruption in
  register_btf_id_dtor_kfuncs() [1]
--

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

bpf: Add flags and release kfuncs to the dtor registry

This commit adds flags and release_btf_id fields to struct btf_id_dtor_kfunc
so the unwinder can look up and validate cleanup destructors alongside map
kptr destructors.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 228b93d4f8c94..6359c5e343bd4 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c

[ ... ]

> @@ -10217,6 +10280,22 @@ static int btf_check_dtor_kfuncs(struct btf *btf, const struct btf_id_dtor_kfunc
>  	for (i = 0; i < cnt; i++) {
>  		dtor_btf_id = btf_relocate_id(btf, dtors[i].kfunc_btf_id);
>  
> +		if (!dtors[i].flags) {
> +			pr_err("no flags for the destructor of btf_id %u\n", dtors[i].btf_id);
> +			return -EINVAL;
> +		}
> +		if ((dtors[i].flags & BPF_DTOR_CLEANUP) && !dtors[i].release_btf_id) {
> +			pr_err("no release kfunc for the cleanup destructor of btf_id %u\n",
> +			       dtors[i].btf_id);
> +			return -EINVAL;
> +		}
> +		if (dtors[i].release_btf_id &&
> +		    btf_check_release_kfunc(btf, btf_relocate_id(btf, dtors[i].release_btf_id))) {
> +			pr_err("btf_id %u is not a release kfunc for the destructor of btf_id %u\n",
> +			       dtors[i].release_btf_id, dtors[i].btf_id);
> +			return -EINVAL;
> +		}
> +
>  		dtor_func = btf_type_by_id(btf, dtor_btf_id);
>  		if (!dtor_func || !btf_type_is_func(dtor_func))
>  			return -EINVAL;
> @@ -10311,8 +10390,12 @@ int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dtors, u32 add_c
>  
>  	/* remap BTF ids based on BTF relocation (if any) */
>  	for (i = tab_cnt; i < tab_cnt + add_cnt; i++) {
> -		tab->dtors[i].btf_id = btf_relocate_id(btf, tab->dtors[i].btf_id);
> -		tab->dtors[i].kfunc_btf_id = btf_relocate_id(btf, tab->dtors[i].kfunc_btf_id);
> +		struct btf_id_dtor_kfunc *dtor = &tab->dtors[i];
> +
> +		dtor->btf_id = btf_relocate_id(btf, dtor->btf_id);
> +		dtor->kfunc_btf_id = btf_relocate_id(btf, dtor->kfunc_btf_id);
> +		if (dtor->release_btf_id)
> +			dtor->release_btf_id = btf_relocate_id(btf, dtor->release_btf_id);
>  	}
>  
>  	tab->cnt += add_cnt;

[Severity: Critical]
This problem wasn't introduced by this patch, but can a failed call to
register_btf_id_dtor_kfuncs() wipe out all previously registered destructors
in the shared vmlinux BTF [1]?

Multiple initcalls append entries to the global vmlinux BTF's dtor_kfunc_tab
via krealloc().  However, if a later registration fails in
btf_check_dtor_kfuncs(), exceeds BTF_DTOR_KFUNC_MAX_CNT, or fails krealloc(),
the error path unconditionally frees the entire table:

kernel/bpf/btf.c:register_btf_id_dtor_kfuncs() {
	...
end:
	if (ret)
		btf_free_dtor_kfunc_tab(btf);
	btf_put(btf);
	return ret;
}

Because the error paths in register_btf_id_dtor_kfuncs() run before the new
entries are appended to btf->dtor_kfunc_tab, dropping the
btf_free_dtor_kfunc_tab(btf) call on error would leave existing vmlinux
entries intact (while module BTFs still free their table in btf_free() if
module init fails).

[1] https://sashiko.dev/#/bug/linux-5f5fa9b2-fc98-4120-b5d5-adb2695404ab

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