Re: [PATCH bpf-next 3/3] selftests/bpf: Add test for tracepoint btf_ids tracefs file

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 5/15/26 7:36 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Missing test environment check for `bpf_testmod` module causes spurious test failures.
> - [Medium] Out-of-bounds array reads and cascading failures due to non-aborting assertions and missing length checks.
> --
> 
> commit 1c2fbe8e9b6c6c1082b4ae630c67ceb3b5db79b5
> Author: Mykyta Yatsenko <yatsenko@meta.com>
> 
> selftests/bpf: Add test for tracepoint btf_ids tracefs file
> 
> This commit adds a selftest to verify that the BTF IDs exposed in the
> tracefs btf_ids file for a module tracepoint match the expected FUNC_PROTO
> and STRUCT representations. It uses the bpf_testmod_test_read tracepoint
> to exercise the module/split-BTF path.
> 
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tp_btf_ids.c b/tools/testing/selftests/bpf/prog_tests/tp_btf_ids.c
>> new file mode 100644
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/tp_btf_ids.c
> 
> [ ... ]
> 
>> +void test_tp_btf_ids(void)
>> +{
>> +	const struct btf_type *proto_t, *rec_t;
>> +	const struct btf_param *params;
>> +	const struct btf_member *members;
>> +	struct btf_ids_info info;
>> +	struct btf *vmlinux_btf, *btf;
>> +	const char *name;
>> +	int err;
>> +
> 
> Since this test relies on bpf_testmod_test_read from the bpf_testmod.ko
> kernel module, should there be a check for env.has_testmod here? 
> 

yes

> Without it, if the module is not loaded, the test might fail with a missing
> tracefs directory instead of gracefully skipping via test__skip().
> 
>> +	err = read_btf_ids(&info);
>> +	if (!ASSERT_OK(err, "read btf_ids"))
>> +		return;
> 
> [ ... ]
> 
>> +	/*
>> +	 * raw_btf_id should be the FUNC_PROTO of __bpf_trace_<call>:
>> +	 *   void *__data, struct task_struct *task,
>> +	 *   struct bpf_testmod_test_read_ctx *ctx
>> +	 */
>> +	proto_t = btf__type_by_id(btf, info.raw_id);
>> +	if (!ASSERT_OK_PTR(proto_t, "raw type_by_id"))
>> +		goto out;
>> +	if (!ASSERT_TRUE(btf_is_func_proto(proto_t), "raw is FUNC_PROTO"))
>> +		goto out;
>> +	ASSERT_EQ(btf_vlen(proto_t), 3, "func_proto arg count");
>> +
>> +	params = btf_params(proto_t);
>> +	ASSERT_STREQ(param_name(btf, &params[0]), "__data", "arg0 name");
>> +	ASSERT_STREQ(param_name(btf, &params[1]), "task", "arg1 name");
>> +	ASSERT_STREQ(param_name(btf, &params[2]), "ctx", "arg2 name");
> 
> Does this code risk reading out of bounds if btf_vlen(proto_t) is less than 3?
> 
> The ASSERT_EQ macro logs a failure but does not inherently abort the
> function execution. Should this be wrapped in a conditional check, such as
> if (!ASSERT_EQ(...)) goto out, to prevent accessing params[2] if the length
> is too small?
> 

ok, I'll apply all these in v2

>> +
>> +	/*
>> +	 * tp_btf_id should be STRUCT trace_event_raw_<call> with the
>> +	 * fields declared by TP_STRUCT__entry plus the common header.
>> +	 */
>> +	rec_t = btf__type_by_id(btf, info.tp_id);
>> +	if (!ASSERT_OK_PTR(rec_t, "tp type_by_id"))
>> +		goto out;
>> +	if (!ASSERT_TRUE(btf_is_struct(rec_t), "tp is STRUCT"))
>> +		goto out;
>> +	name = btf__name_by_offset(btf, rec_t->name_off);
>> +	ASSERT_STREQ(name, "trace_event_raw_bpf_testmod_test_read",
>> +		     "tp struct name");
>> +
>> +	members = btf_members(rec_t);
>> +	ASSERT_STREQ(member_name(btf, &members[0]), "ent", "field0 name");
>> +	ASSERT_STREQ(member_name(btf, &members[1]), "pid", "field1 name");
>> +	ASSERT_STREQ(member_name(btf, &members[2]), "comm", "field2 name");
>> +	ASSERT_STREQ(member_name(btf, &members[3]), "off", "field3 name");
>> +	ASSERT_STREQ(member_name(btf, &members[4]), "len", "field4 name");
> 
> Similarly, is it necessary to assert btf_vlen(rec_t) and conditionally jump
> to out if it is too small before accessing members[0] through members[4]?
> 
> If the structure layout changes or is malformed, this might read adjacent
> BTF data.
> 
>> +out:
>> +	btf__free(btf);
>> +	btf__free(vmlinux_btf);
>> +}
>