Re: [PATCH v1 bpf-next 02/18] bpf: Add a btf_id_set8_entry helper

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/11/06 08:58PM, sashiko-bot@kernel.org wrote:
> 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:
> - [High] bpf: kfunc sets destruction on failure in
>   btf_populate_kfunc_set() [1]
> --
> 
> commit 0f99bfedbc98b8fbe7b7a25dd4038c3e714adb73
> Author: Anton Protopopov <a.s.protopopov@gmail.com>
> 
> bpf: Add a btf_id_set8_entry helper
> 
> This commit names the element type of struct btf_id_set8 as struct
> btf_id_set8_pair, introduces btf_id_set8_entry() to look up a pair, and
> changes btf_id_set8_contains() to return a boolean.
> 
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 228b93d4f8c94..61d2df6776a99 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -9973,8 +9973,8 @@ static u32 *btf_kfunc_id_set_contains(const struct btf *btf,
> >  				      enum btf_kfunc_hook hook,
> >  				      u32 kfunc_btf_id)
> >  {
> > +	struct btf_id_set8_pair *pair;
> >  	struct btf_id_set8 *set;
> > -	u32 *id;
> >  
> >  	if (hook >= BTF_KFUNC_HOOK_MAX)
> >  		return NULL;
> > @@ -9983,11 +9983,10 @@ static u32 *btf_kfunc_id_set_contains(const struct btf *btf,
> >  	set = btf->kfunc_set_tab->sets[hook];
> >  	if (!set)
> >  		return NULL;
> > -	id = btf_id_set8_contains(set, kfunc_btf_id);
> > -	if (!id)
> > +	pair = btf_id_set8_entry(set, kfunc_btf_id);
> > +	if (!pair)
> >  		return NULL;
> > -	/* The flags for BTF ID are located next to it */
> > -	return id + 1;
> > +	return &pair->flags;
> >  }
> 
> [Severity: High]
> This problem wasn't introduced by this patch, but...

Maybe we can do a separate sink for such reports?  Or some tagging such that
this is easier to follow up later?

> btf_populate_kfunc_set() function that populates btf->kfunc_set_tab, does an
> error on a later registration destroy all previously registered kfunc sets
> [1]?
> 
> kernel/bpf/btf.c:btf_populate_kfunc_set() {
>     ...
> 	/* Grow set */
> 	set = krealloc(tab->sets[hook],
> 		       struct_size(set, pairs, set_cnt + add_set->cnt),
> 		       GFP_KERNEL | __GFP_NOWARN);
> 	if (!set) {
> 		ret = -ENOMEM;
> 		goto end;
> 	}
>     ...
> end:
> 	btf_free_kfunc_set_tab(btf);
> 	return ret;
> }
> 
> Multiple subsystems register kfunc sets into the shared vmlinux BTF.  If a
> later registration fails in krealloc() or exceeds BTF_KFUNC_SET_MAX_CNT,
> jumping to end calls btf_free_kfunc_set_tab(), which frees every hook's set
> and clears btf->kfunc_set_tab, breaking all previously registered kfuncs.

For the error itself. The only non-deterministic way to fail here
is with -ENOMEM (on boot, which is not realistic). Other ways to fail
will fail during tests. So any cleanup looks "reasonable".

> [1] https://sashiko.dev/#/bug/linux-1a311d3c-46c9-4fa6-a26e-40badcd832d8
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261106205523.1765622-1-a.s.protopopov@gmail.com?part=2