> On Nov 2, 2023, at 11:07 AM, Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
>
>> On Thu, Nov 2, 2023 at 10:55 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>>
>>> Andrii Nakryiko <andrii@xxxxxxxxxx> writes:
>>>
>>>> Let's fix it for real this time. It shouldn't just detect ERR_PTR()
>>>> return from bpf_xdp_pointer(), but also turn that into NULL to follow
>>>> bpf_dynptr_slice() contract.
>>>>
>>>> Fixes: 5426700e6841 ("bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.")
>>>> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
>>>> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>>>> ---
>>>> kernel/bpf/helpers.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>>> index 56b0c1f678ee..04049097176c 100644
>>>> --- a/kernel/bpf/helpers.c
>>>> +++ b/kernel/bpf/helpers.c
>>>> @@ -2309,7 +2309,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
>>>> {
>>>> void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);
>>>> if (!IS_ERR_OR_NULL(xdp_ptr))
>>>> - return xdp_ptr;
>>>> + return NULL;
>>>
>>> Erm, the check in the if is inverted - so isn't this 'return xdp_ptr'
>>> covering the case where bpf_xdp_pointer() *does* in fact return a valid
>>> pointer?
>>>
>>
>> Ah, you are right, I missed the ! part... Ok, then I don't think we
>> have an issue, great. Thanks for double checking!
>> Perhaps we should add a simple comment "/* we got a valid direct
>> pointer, return it */", as this looks like an error-handling case.
>
> Yup, totally agree it's confusing, I had to look at the code three or
> four times as well just now, to be sure that it wasn't buggy. Adding a
> comment would certainly be useful! :)
Aha, I was confused by this for more than a month. I am glad this is
not an issue.
Thanks,
Song
