Re: [PATCHv5 bpf-next 21/28] selftests/bpf: Add tracing multi skel/pattern/ids attach tests

Re: [PATCHv5 bpf-next 21/28] selftests/bpf: Add tracing multi skel/pattern/ids attach tests

[Jiri Olsa]

On Sat, Apr 18, 2026 at 03:34:37AM +0000, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 2 potential issue(s):
> - [Medium] Double free of `vmlinux_btf` in the error path of `get_ids()`.
> - [Medium] Missing error state assignment in `get_ids()` leads to returning a partially or fully zero-initialized array instead of failing.
> --
> 
> commit 44b619347c03e0f87cfb994ba8b14346b293edcc
> Author: Jiri Olsa <jolsa@kernel.org>
> 
> selftests/bpf: Add tracing multi skel/pattern/ids attach tests
> 
> Adding tests for tracing_multi link attachment via all possible
> libbpf apis - skeleton, function pattern and btf ids.
> 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_multi.c b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> > new file mode 100644
> > index 0000000000000..52d3339b430a5
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> 
> [ ... ]
> 
> > +static __u32 *get_ids(const char * const funcs[], int funcs_cnt, const char *mod)
> > +{
> > +	struct btf *btf, *vmlinux_btf = NULL;
> > +	__u32 nr, type_id, cnt = 0;
> > +	void *root = NULL;
> > +	__u32 *ids = NULL;
> > +	int i, err = 0;
> > +
> > +	btf = btf__load_vmlinux_btf();
> > +	if (!ASSERT_OK_PTR(btf, "btf__load_vmlinux_btf"))
> > +		return NULL;
> > +
> > +	if (mod) {
> > +		vmlinux_btf = btf;
> > +		btf = btf__load_module_btf(mod, vmlinux_btf);
> > +		if (!ASSERT_OK_PTR(btf, "btf__load_module_btf")) {
> > +			btf__free(vmlinux_btf);
> > +			goto out;
> 
> Can this result in a double free of vmlinux_btf? If btf__load_module_btf()
> fails, we call btf__free(vmlinux_btf) here, and then goto out, where
> btf__free(vmlinux_btf) is called again.

replied for this one in other email

> 
> > +		}
> > +	}
> > +
> > +	ids = calloc(funcs_cnt, sizeof(ids[0]));
> > +	if (!ids)
> > +		goto out;
> > +
> > +	/*
> > +	 * We sort function names by name and search them
> > +	 * below for each function.
> > +	 */
> > +	for (i = 0; i < funcs_cnt; i++) {
> > +		if (!tsearch(&funcs[i], &root, compare)) {
> > +			ASSERT_FAIL("tsearch failed");
> > +			goto out;
> 
> Does this code return an invalid array if tsearch() fails? When tsearch()
> fails, we jump to out, but err is still 0. Because err is 0, the cleanup
> block below won't free ids and will instead return the partially or fully
> zero-initialized array.
> 
> Also, if the BTF type loop that follows this completes without finding all
> expected functions (cnt < funcs_cnt), err also remains 0.
> 
> Should err be set to a non-zero value before the goto and when
> cnt < funcs_cnt?

yep, I need to set err properly

jirka