Re: [PATCH bpf-next v2 2/4] bpf: selftests: Check for flag presence in bpf_program__flags()

["Toke Høiland-Jørgensen" <toke@redhat.com>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Missing NULL pointer check on the result of bpf_object__next_program() causes a potential test crash.
> --
>
> commit 357b58adee859a9375d4063d6f64aa4a923e7790
> Author: Toke Høiland-Jørgensen <toke@redhat.com>
>
> bpf: selftests: Check for flag presence in bpf_program__flags()
>
> This commit adds checks to verify that the BPF_F_XDP_HAS_FRAGS and
> BPF_F_SLEEPABLE flags are correctly exposed by bpf_program__flags(). It
> introduces assertions in multiple selftests immediately after retrieving
> the BPF program structures.
>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c
>> index fce203640f8cf..a894b1ab46f4e 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c
>> @@ -18,6 +18,9 @@ static void test_xdp_update_frags(void)
>>  		return;
>>  
>>  	prog = bpf_object__next_program(obj, NULL);
>> +	ASSERT_EQ(bpf_program__flags(prog) & BPF_F_XDP_HAS_FRAGS,
>> +		  BPF_F_XDP_HAS_FRAGS, "frags in program flags");
>> +
>
> [Severity: Medium]
> What happens if bpf_object__next_program() returns a NULL pointer here?
>
> The prog pointer is passed directly to bpf_program__flags(), which
> unconditionally dereferences it. While the object file is expected to contain
> programs during normal builds, if it does not, could this cause the test suite
> to hard crash with a segmentation fault instead of failing gracefully?
>
> Prior to this patch, a NULL prog would be handled more safely, as the later
> call to bpf_program__fd() explicitly checks for NULL and returns an error
> code. 
>
> Does it make sense to add a check like ASSERT_OK_PTR(prog, "prog") before
> accessing its flags?

No. This won't happen, and existing code already use the pointer without
checking it.

-Toke