[PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[Felix Maurer]

If bpf_msg_push_data is called with len 0 (as it happens during
selftests/bpf/test_sockmap), we do not need to do anything and can
return early.

Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
error: we later called get_order(copy + len); if len was 0, copy + len
was also often 0 and get_order returned some undefined value (at the
moment 52). alloc_pages caught that and failed, but then
bpf_msg_push_data returned ENOMEM. This was wrong because we are most
probably not out of memory and actually do not need any additional
memory.

v2: Add bug description and Fixes tag

Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data")
Signed-off-by: Felix Maurer <fmaurer@redhat.com>
---
 net/core/filter.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 4603b7cd3cd1..9eb785842258 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2710,6 +2710,9 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
 	if (unlikely(flags))
 		return -EINVAL;
 
+	if (unlikely(len == 0))
+		return 0;
+
 	/* First find the starting scatterlist element */
 	i = msg->sg.start;
 	do {
-- 
2.34.1

Re: [PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[Yonghong Song]

On 2/9/22 7:55 AM, Felix Maurer wrote:
> If bpf_msg_push_data is called with len 0 (as it happens during
> selftests/bpf/test_sockmap), we do not need to do anything and can
> return early.
> 
> Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
> error: we later called get_order(copy + len); if len was 0, copy + len
> was also often 0 and get_order returned some undefined value (at the
> moment 52). alloc_pages caught that and failed, but then
> bpf_msg_push_data returned ENOMEM. This was wrong because we are most
> probably not out of memory and actually do not need any additional
> memory.
> 
> v2: Add bug description and Fixes tag
> 
> Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data")
> Signed-off-by: Felix Maurer <fmaurer@redhat.com>

LGTM. I am wondering why bpf CI didn't catch this problem. Did you
modified the test with length 0 in order to trigger that? If this
is the case, it would be great you can add such a test to the
test_sockmap.

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[Felix Maurer]

On 09.02.22 18:06, Yonghong Song wrote:
> On 2/9/22 7:55 AM, Felix Maurer wrote:
>> If bpf_msg_push_data is called with len 0 (as it happens during
>> selftests/bpf/test_sockmap), we do not need to do anything and can
>> return early.
>>
>> Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
>> error: we later called get_order(copy + len); if len was 0, copy + len
>> was also often 0 and get_order returned some undefined value (at the
>> moment 52). alloc_pages caught that and failed, but then
>> bpf_msg_push_data returned ENOMEM. This was wrong because we are most
>> probably not out of memory and actually do not need any additional
>> memory.
>>
>> v2: Add bug description and Fixes tag
>>
>> Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data")
>> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
> 
> LGTM. I am wondering why bpf CI didn't catch this problem. Did you
> modified the test with length 0 in order to trigger that? If this
> is the case, it would be great you can add such a test to the
> test_sockmap.

I did not modify the tests to trigger that. The state of the selftests
around that is unfortunately not very good. There is no explicit test
with length 0 but bpf_msg_push_data is still called with length 0,
because of what I consider to be bugs in the test. On the other hand,
explicit tests with other lengths are sometimes not called as well. I'll
elaborate on that in a bit.

Something easy to fix is that the tests do not check the return value of
bpf_msg_push_data which they probably should. That may have helped find
the problem earlier.

Now to the issue mentioned in the beginning: Only some of the BPF
programs used in test_sockmap actually call bpf_msg_push_data. However,
they are not always attached, just for particular scenarios:
txmsg_pass==1, txmsg_redir==1, or txmsg_drop==1. If none of those apply,
bpf_msg_push_data is never called. This happens for example in
test_txmsg_push. Out of the four defined tests only one actually calls
the helper.

But after a test, the parameters in the map are reset to 0 (instead of
being removed). Therefore, when the maps are reused in a subsequent test
which is one of the scenarios above, the values are present and
bpf_msg_push_data is called, albeit with the parameters set to 0. This
is also what triggered the wrong behavior fixed in the patch.

Unfortunately, I do not have the time to fix these issues in the test at
the moment.

> Acked-by: Yonghong Song <yhs@fb.com>

Thanks!

Re: [PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[Yonghong Song]

On 2/10/22 7:45 AM, Felix Maurer wrote:
> On 09.02.22 18:06, Yonghong Song wrote:
>> On 2/9/22 7:55 AM, Felix Maurer wrote:
>>> If bpf_msg_push_data is called with len 0 (as it happens during
>>> selftests/bpf/test_sockmap), we do not need to do anything and can
>>> return early.
>>>
>>> Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
>>> error: we later called get_order(copy + len); if len was 0, copy + len
>>> was also often 0 and get_order returned some undefined value (at the
>>> moment 52). alloc_pages caught that and failed, but then
>>> bpf_msg_push_data returned ENOMEM. This was wrong because we are most
>>> probably not out of memory and actually do not need any additional
>>> memory.
>>>
>>> v2: Add bug description and Fixes tag
>>>
>>> Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data")
>>> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
>>
>> LGTM. I am wondering why bpf CI didn't catch this problem. Did you
>> modified the test with length 0 in order to trigger that? If this
>> is the case, it would be great you can add such a test to the
>> test_sockmap.
> 
> I did not modify the tests to trigger that. The state of the selftests
> around that is unfortunately not very good. There is no explicit test
> with length 0 but bpf_msg_push_data is still called with length 0,
> because of what I consider to be bugs in the test. On the other hand,
> explicit tests with other lengths are sometimes not called as well. I'll
> elaborate on that in a bit.
> 
> Something easy to fix is that the tests do not check the return value of
> bpf_msg_push_data which they probably should. That may have helped find
> the problem earlier.
> 
> Now to the issue mentioned in the beginning: Only some of the BPF
> programs used in test_sockmap actually call bpf_msg_push_data. However,
> they are not always attached, just for particular scenarios:
> txmsg_pass==1, txmsg_redir==1, or txmsg_drop==1. If none of those apply,
> bpf_msg_push_data is never called. This happens for example in
> test_txmsg_push. Out of the four defined tests only one actually calls
> the helper.
> 
> But after a test, the parameters in the map are reset to 0 (instead of
> being removed). Therefore, when the maps are reused in a subsequent test
> which is one of the scenarios above, the values are present and
> bpf_msg_push_data is called, albeit with the parameters set to 0. This
> is also what triggered the wrong behavior fixed in the patch.
> 
> Unfortunately, I do not have the time to fix these issues in the test at
> the moment.

Thanks for detailed explanation. Maybe for the immediate case, can you
just fix this in the selftest,

   > Something easy to fix is that the tests do not check the return 
value of
   > bpf_msg_push_data which they probably should. That may have helped find
   > the problem earlier.

This will be enough to verify your kernel change as without it the
test will fail.

The rest of test improvements can come later.

> 
>> Acked-by: Yonghong Song <yhs@fb.com>
> 
> Thanks!
> 

Re: [PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[Alexei Starovoitov]

On Thu, Feb 10, 2022 at 10:05 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 2/10/22 7:45 AM, Felix Maurer wrote:
> > On 09.02.22 18:06, Yonghong Song wrote:
> >> On 2/9/22 7:55 AM, Felix Maurer wrote:
> >>> If bpf_msg_push_data is called with len 0 (as it happens during
> >>> selftests/bpf/test_sockmap), we do not need to do anything and can
> >>> return early.
> >>>
> >>> Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
> >>> error: we later called get_order(copy + len); if len was 0, copy + len
> >>> was also often 0 and get_order returned some undefined value (at the
> >>> moment 52). alloc_pages caught that and failed, but then
> >>> bpf_msg_push_data returned ENOMEM. This was wrong because we are most
> >>> probably not out of memory and actually do not need any additional
> >>> memory.
> >>>
> >>> v2: Add bug description and Fixes tag
> >>>
> >>> Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data")
> >>> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
> >>
> >> LGTM. I am wondering why bpf CI didn't catch this problem. Did you
> >> modified the test with length 0 in order to trigger that? If this
> >> is the case, it would be great you can add such a test to the
> >> test_sockmap.
> >
> > I did not modify the tests to trigger that. The state of the selftests
> > around that is unfortunately not very good. There is no explicit test
> > with length 0 but bpf_msg_push_data is still called with length 0,
> > because of what I consider to be bugs in the test. On the other hand,
> > explicit tests with other lengths are sometimes not called as well. I'll
> > elaborate on that in a bit.
> >
> > Something easy to fix is that the tests do not check the return value of
> > bpf_msg_push_data which they probably should. That may have helped find
> > the problem earlier.
> >
> > Now to the issue mentioned in the beginning: Only some of the BPF
> > programs used in test_sockmap actually call bpf_msg_push_data. However,
> > they are not always attached, just for particular scenarios:
> > txmsg_pass==1, txmsg_redir==1, or txmsg_drop==1. If none of those apply,
> > bpf_msg_push_data is never called. This happens for example in
> > test_txmsg_push. Out of the four defined tests only one actually calls
> > the helper.
> >
> > But after a test, the parameters in the map are reset to 0 (instead of
> > being removed). Therefore, when the maps are reused in a subsequent test
> > which is one of the scenarios above, the values are present and
> > bpf_msg_push_data is called, albeit with the parameters set to 0. This
> > is also what triggered the wrong behavior fixed in the patch.
> >
> > Unfortunately, I do not have the time to fix these issues in the test at
> > the moment.
>
> Thanks for detailed explanation. Maybe for the immediate case, can you
> just fix this in the selftest,
>
>    > Something easy to fix is that the tests do not check the return
> value of
>    > bpf_msg_push_data which they probably should. That may have helped find
>    > the problem earlier.
>
> This will be enough to verify your kernel change as without it the
> test will fail.
>
> The rest of test improvements can come later.

John,
what is your take on this fix?
bpf tree material?

Re: [PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[John Fastabend]

Alexei Starovoitov wrote:
> On Thu, Feb 10, 2022 at 10:05 AM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 2/10/22 7:45 AM, Felix Maurer wrote:
> > > On 09.02.22 18:06, Yonghong Song wrote:
> > >> On 2/9/22 7:55 AM, Felix Maurer wrote:
> > >>> If bpf_msg_push_data is called with len 0 (as it happens during
> > >>> selftests/bpf/test_sockmap), we do not need to do anything and can
> > >>> return early.
> > >>>
> > >>> Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
> > >>> error: we later called get_order(copy + len); if len was 0, copy + len
> > >>> was also often 0 and get_order returned some undefined value (at the
> > >>> moment 52). alloc_pages caught that and failed, but then
> > >>> bpf_msg_push_data returned ENOMEM. This was wrong because we are most
> > >>> probably not out of memory and actually do not need any additional
> > >>> memory.
> > >>>
> > >>> v2: Add bug description and Fixes tag
> > >>>
> > >>> Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data")
> > >>> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
> > >>
> > >> LGTM. I am wondering why bpf CI didn't catch this problem. Did you
> > >> modified the test with length 0 in order to trigger that? If this
> > >> is the case, it would be great you can add such a test to the
> > >> test_sockmap.
> > >
> > > I did not modify the tests to trigger that. The state of the selftests
> > > around that is unfortunately not very good. There is no explicit test
> > > with length 0 but bpf_msg_push_data is still called with length 0,
> > > because of what I consider to be bugs in the test. On the other hand,
> > > explicit tests with other lengths are sometimes not called as well. I'll
> > > elaborate on that in a bit.
> > >
> > > Something easy to fix is that the tests do not check the return value of
> > > bpf_msg_push_data which they probably should. That may have helped find
> > > the problem earlier.
> > >
> > > Now to the issue mentioned in the beginning: Only some of the BPF
> > > programs used in test_sockmap actually call bpf_msg_push_data. However,
> > > they are not always attached, just for particular scenarios:
> > > txmsg_pass==1, txmsg_redir==1, or txmsg_drop==1. If none of those apply,
> > > bpf_msg_push_data is never called. This happens for example in
> > > test_txmsg_push. Out of the four defined tests only one actually calls
> > > the helper.
> > >
> > > But after a test, the parameters in the map are reset to 0 (instead of
> > > being removed). Therefore, when the maps are reused in a subsequent test
> > > which is one of the scenarios above, the values are present and
> > > bpf_msg_push_data is called, albeit with the parameters set to 0. This
> > > is also what triggered the wrong behavior fixed in the patch.
> > >
> > > Unfortunately, I do not have the time to fix these issues in the test at
> > > the moment.
> >
> > Thanks for detailed explanation. Maybe for the immediate case, can you
> > just fix this in the selftest,
> >
> >    > Something easy to fix is that the tests do not check the return
> > value of
> >    > bpf_msg_push_data which they probably should. That may have helped find
> >    > the problem earlier.
> >
> > This will be enough to verify your kernel change as without it the
> > test will fail.
> >
> > The rest of test improvements can come later.
> 
> John,
> what is your take on this fix?

Fix looks good its nice to return 0 here instead of ENOMEM as a result
of paticulars of passing 0 to get_order(). Ack for me.

> bpf tree material?

I checked our code here and we would never pass '0' to pull data. Its hard
to imagine what type of code would do that, but on the other hand its a
bug and its only rc3... I've no strong opinion if I wrote the patch I would
have pointed it at bpf tree so slight preference to push it as a fix.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[Felix Maurer]

On 10.02.22 19:04, Yonghong Song wrote:
> On 2/10/22 7:45 AM, Felix Maurer wrote:
>> On 09.02.22 18:06, Yonghong Song wrote:
>>> On 2/9/22 7:55 AM, Felix Maurer wrote:
>>>> If bpf_msg_push_data is called with len 0 (as it happens during
>>>> selftests/bpf/test_sockmap), we do not need to do anything and can
>>>> return early.
>>>>
>>>> Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
>>>> error: we later called get_order(copy + len); if len was 0, copy + len
>>>> was also often 0 and get_order returned some undefined value (at the
>>>> moment 52). alloc_pages caught that and failed, but then
>>>> bpf_msg_push_data returned ENOMEM. This was wrong because we are most
>>>> probably not out of memory and actually do not need any additional
>>>> memory.
>>>>
>>>> v2: Add bug description and Fixes tag
>>>>
>>>> Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data")
>>>> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
>>>
>>> LGTM. I am wondering why bpf CI didn't catch this problem. Did you
>>> modified the test with length 0 in order to trigger that? If this
>>> is the case, it would be great you can add such a test to the
>>> test_sockmap.
>>
>> I did not modify the tests to trigger that. The state of the selftests
>> around that is unfortunately not very good. There is no explicit test
>> with length 0 but bpf_msg_push_data is still called with length 0,
>> because of what I consider to be bugs in the test. On the other hand,
>> explicit tests with other lengths are sometimes not called as well. I'll
>> elaborate on that in a bit.
>>
>> Something easy to fix is that the tests do not check the return value of
>> bpf_msg_push_data which they probably should. That may have helped find
>> the problem earlier.
>>
>> Now to the issue mentioned in the beginning: Only some of the BPF
>> programs used in test_sockmap actually call bpf_msg_push_data. However,
>> they are not always attached, just for particular scenarios:
>> txmsg_pass==1, txmsg_redir==1, or txmsg_drop==1. If none of those apply,
>> bpf_msg_push_data is never called. This happens for example in
>> test_txmsg_push. Out of the four defined tests only one actually calls
>> the helper.
>>
>> But after a test, the parameters in the map are reset to 0 (instead of
>> being removed). Therefore, when the maps are reused in a subsequent test
>> which is one of the scenarios above, the values are present and
>> bpf_msg_push_data is called, albeit with the parameters set to 0. This
>> is also what triggered the wrong behavior fixed in the patch.
>>
>> Unfortunately, I do not have the time to fix these issues in the test at
>> the moment.
> 
> Thanks for detailed explanation. Maybe for the immediate case, can you
> just fix this in the selftest,
> 
>   > Something easy to fix is that the tests do not check the return
> value of
>   > bpf_msg_push_data which they probably should. That may have helped find
>   > the problem earlier.
> 
> This will be enough to verify your kernel change as without it the
> test will fail.

I just send a patch checking the return values of the bpf_msg_push_data
usages in the test [1]. Passing the errors to userspace by dropping
packets is not very nice, but a straightforward way in the current test
program.

I did try the same checks of the return values of bpf_msg_pull_data, but
then the tests fail. So there might be something hidden here as well.

[1]:https://lore.kernel.org/bpf/89f767bb44005d6b4dd1f42038c438f76b3ebfad.1644601294.git.fmaurer@redhat.com/

> The rest of test improvements can come later.
> 
>>
>>> Acked-by: Yonghong Song <yhs@fb.com>
>>
>> Thanks!
>>
> 

Re: [PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[Yonghong Song]

On 2/11/22 9:52 AM, Felix Maurer wrote:
> On 10.02.22 19:04, Yonghong Song wrote:
>> On 2/10/22 7:45 AM, Felix Maurer wrote:
>>> On 09.02.22 18:06, Yonghong Song wrote:
>>>> On 2/9/22 7:55 AM, Felix Maurer wrote:
>>>>> If bpf_msg_push_data is called with len 0 (as it happens during
>>>>> selftests/bpf/test_sockmap), we do not need to do anything and can
>>>>> return early.
>>>>>
>>>>> Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
>>>>> error: we later called get_order(copy + len); if len was 0, copy + len
>>>>> was also often 0 and get_order returned some undefined value (at the
>>>>> moment 52). alloc_pages caught that and failed, but then
>>>>> bpf_msg_push_data returned ENOMEM. This was wrong because we are most
>>>>> probably not out of memory and actually do not need any additional
>>>>> memory.
>>>>>
>>>>> v2: Add bug description and Fixes tag
>>>>>
>>>>> Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data")
>>>>> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
>>>>
>>>> LGTM. I am wondering why bpf CI didn't catch this problem. Did you
>>>> modified the test with length 0 in order to trigger that? If this
>>>> is the case, it would be great you can add such a test to the
>>>> test_sockmap.
>>>
>>> I did not modify the tests to trigger that. The state of the selftests
>>> around that is unfortunately not very good. There is no explicit test
>>> with length 0 but bpf_msg_push_data is still called with length 0,
>>> because of what I consider to be bugs in the test. On the other hand,
>>> explicit tests with other lengths are sometimes not called as well. I'll
>>> elaborate on that in a bit.
>>>
>>> Something easy to fix is that the tests do not check the return value of
>>> bpf_msg_push_data which they probably should. That may have helped find
>>> the problem earlier.
>>>
>>> Now to the issue mentioned in the beginning: Only some of the BPF
>>> programs used in test_sockmap actually call bpf_msg_push_data. However,
>>> they are not always attached, just for particular scenarios:
>>> txmsg_pass==1, txmsg_redir==1, or txmsg_drop==1. If none of those apply,
>>> bpf_msg_push_data is never called. This happens for example in
>>> test_txmsg_push. Out of the four defined tests only one actually calls
>>> the helper.
>>>
>>> But after a test, the parameters in the map are reset to 0 (instead of
>>> being removed). Therefore, when the maps are reused in a subsequent test
>>> which is one of the scenarios above, the values are present and
>>> bpf_msg_push_data is called, albeit with the parameters set to 0. This
>>> is also what triggered the wrong behavior fixed in the patch.
>>>
>>> Unfortunately, I do not have the time to fix these issues in the test at
>>> the moment.
>>
>> Thanks for detailed explanation. Maybe for the immediate case, can you
>> just fix this in the selftest,
>>
>>    > Something easy to fix is that the tests do not check the return
>> value of
>>    > bpf_msg_push_data which they probably should. That may have helped find
>>    > the problem earlier.
>>
>> This will be enough to verify your kernel change as without it the
>> test will fail.
> 
> I just send a patch checking the return values of the bpf_msg_push_data
> usages in the test [1]. Passing the errors to userspace by dropping
> packets is not very nice, but a straightforward way in the current test
> program.
> 
> I did try the same checks of the return values of bpf_msg_pull_data, but
> then the tests fail. So there might be something hidden here as well.

Thanks for the patch! Maybe this can be the first step to fix
test_sockmap.

John, could you help take a look at the patch?

> 
> [1]:https://lore.kernel.org/bpf/89f767bb44005d6b4dd1f42038c438f76b3ebfad.1644601294.git.fmaurer@redhat.com/
> 
>> The rest of test improvements can come later.
>>
>>>
>>>> Acked-by: Yonghong Song <yhs@fb.com>
>>>
>>> Thanks!
>>>
>>
> 

Re: [PATCH bpf-next v2] bpf: Do not try bpf_msg_push_data with len 0

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Wed,  9 Feb 2022 16:55:26 +0100 you wrote:
> If bpf_msg_push_data is called with len 0 (as it happens during
> selftests/bpf/test_sockmap), we do not need to do anything and can
> return early.
> 
> Calling bpf_msg_push_data with len 0 previously lead to a wrong ENOMEM
> error: we later called get_order(copy + len); if len was 0, copy + len
> was also often 0 and get_order returned some undefined value (at the
> moment 52). alloc_pages caught that and failed, but then
> bpf_msg_push_data returned ENOMEM. This was wrong because we are most
> probably not out of memory and actually do not need any additional
> memory.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2] bpf: Do not try bpf_msg_push_data with len 0
    https://git.kernel.org/bpf/bpf/c/4a11678f6838

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html