[PATCH bpf] bpf: fix bpf_dynptr_slice() returning ERR_PTR() on erro

[PATCH bpf] bpf: fix bpf_dynptr_slice() returning ERR_PTR() on erro

[Andrii Nakryiko]

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@kernel.org>
---
 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;
 
 		if (!buffer__opt)
 			return NULL;
-- 
2.34.1

Re: [PATCH bpf] bpf: fix bpf_dynptr_slice() returning ERR_PTR() on erro

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii@kernel.org> 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@kernel.org>
> ---
>  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?

-Toke

Re: [PATCH bpf] bpf: fix bpf_dynptr_slice() returning ERR_PTR() on erro

[Andrii Nakryiko]

On Thu, Nov 2, 2023 at 10:55 AM Toke Høiland-Jørgensen <toke@kernel.org> wrote:
>
> Andrii Nakryiko <andrii@kernel.org> 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@kernel.org>
> > ---
> >  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.

> -Toke

Re: [PATCH bpf] bpf: fix bpf_dynptr_slice() returning ERR_PTR() on erro

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Nov 2, 2023 at 10:55 AM Toke Høiland-Jørgensen <toke@kernel.org> wrote:
>>
>> Andrii Nakryiko <andrii@kernel.org> 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@kernel.org>
>> > ---
>> >  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! :)

-Toke

Re: [PATCH bpf] bpf: fix bpf_dynptr_slice() returning ERR_PTR() on erro

[Song Liu]

> On Nov 2, 2023, at 11:07 AM, Toke Høiland-Jørgensen <toke@kernel.org> wrote:
> 
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> 
>> On Thu, Nov 2, 2023 at 10:55 AM Toke Høiland-Jørgensen <toke@kernel.org> wrote:
>>> 
>>> Andrii Nakryiko <andrii@kernel.org> 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@kernel.org>
>>>> ---
>>>> 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