Re: [PATCH bpf-next v2 3/6] selftests/bpf: Verify arena type tags in resolve_btfids test

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing NULL pointer check on the result of `btf__type_by_id()` causes a segmentation fault when a kfunc symbol is missing.
--

--- Patch [3]: [PATCH bpf-next v2 3/6] selftests/bpf: Verify arena type tags in resolve_btfids test ---
commit 833ac8bbb8ac1055e25790da1396864987e31541
Author: Ihor Solodrai <[email protected]>

selftests/bpf: Verify arena type tags in resolve_btfids test

This commit extends test_resolve_btfids() to assert that resolve_btfids
emits the address_space(1) type attribute on the return type and arguments
of kfuncs marked with KF_ARENA_RET, KF_ARENA_ARG1 or KF_ARENA_ARG2.

> diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> index ac51fd4548214..8482f00046d4c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> +++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
[ ... ]
> @@ -227,6 +260,39 @@ void test_resolve_btfids(void)
>  	check_kfunc_set(&test_kfunc_set);
>  	check_kfunc_set(&test_kfunc_set_rev);
>  
> +	/*
> +	 * Check resolve_btfids wrapped exactly the arena-flagged return/args
> +	 * with the address_space(1) type attribute, and left other
> +	 * pointers/returns untouched.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(kfunc_symbols); i++) {
> +		const struct btf_type *fn, *proto;
> +		const struct btf_param *params;
> +		const char *name = kfunc_symbols[i].name;
> +		u32 fl = kfunc_symbols[i].flags;
> +		__u32 nr;
> +
> +		fn = btf__type_by_id(btf, kfunc_symbols[i].id);
> +		if (!ASSERT_TRUE(btf_is_func(fn), name))
> +			continue;

[Severity: Medium]
Could this cause a segmentation fault if a kfunc symbol is missing?

If resolve_symbols() fails to find a kfunc in the parsed BTF file, the
symbol's id remains -1. When -1 is passed to btf__type_by_id(), it
returns NULL.

Since btf_is_func() unconditionally accesses the info member, passing a
NULL pointer to it would crash the test program before the ASSERT_TRUE
evaluates.

Should we check if fn is NULL before passing it to btf_is_func()?

> +		proto = btf__type_by_id(btf, fn->type);
> +		if (!ASSERT_TRUE(btf_is_func_proto(proto), name))
> +			continue;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3