[PATCH bpf 0/3] s390/bpf: Fix gotol with large offsets

[PATCH bpf 0/3] s390/bpf: Fix gotol with large offsets

[Ilya Leoshkevich]

Hi,

While looking at a pyperf180 failure on s390x (must be related to [1],
I'm not done with the investigation yet) I noticed that I have
unfortunately messed up the gotol implementation. Patch 1 is the fix,
patch 2 is a small test infrastructure tweak, and patch 3 adds a
test.

[1] https://github.com/llvm/llvm-project/issues/55669

Best regards,
Ilya

Ilya Leoshkevich (3):
  s390/bpf: Fix gotol with large offsets
  selftests/bpf: Double the size of test_loader log
  selftests/bpf: Test gotol with large offsets

 arch/s390/net/bpf_jit_comp.c                  |  2 +-
 .../selftests/bpf/progs/verifier_gotol.c      | 19 +++++++++++++++++++
 tools/testing/selftests/bpf/test_loader.c     |  2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH bpf 1/3] s390/bpf: Fix gotol with large offsets

[Ilya Leoshkevich]

The gotol implementation uses a wrong data type for the offset: it
should be s32, not s16.

Fixes: c690191e23d8 ("s390/bpf: Implement unconditional jump with 32-bit offset")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index bf06b7283f0c..c7fbeedeb0a4 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -779,7 +779,7 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 				 int i, bool extra_pass, u32 stack_depth)
 {
 	struct bpf_insn *insn = &fp->insnsi[i];
-	s16 branch_oc_off = insn->off;
+	s32 branch_oc_off = insn->off;
 	u32 dst_reg = insn->dst_reg;
 	u32 src_reg = insn->src_reg;
 	int last, insn_count = 1;
-- 
2.43.0

Re: [PATCH bpf 1/3] s390/bpf: Fix gotol with large offsets

[Yonghong Song]

On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> The gotol implementation uses a wrong data type for the offset: it
> should be s32, not s16.
>
> Fixes: c690191e23d8 ("s390/bpf: Implement unconditional jump with 32-bit offset")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

RE: [PATCH bpf 1/3] s390/bpf: Fix gotol with large offsets

[John Fastabend]

Ilya Leoshkevich wrote:
> The gotol implementation uses a wrong data type for the offset: it
> should be s32, not s16.
> 
> Fixes: c690191e23d8 ("s390/bpf: Implement unconditional jump with 32-bit offset")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  arch/s390/net/bpf_jit_comp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index bf06b7283f0c..c7fbeedeb0a4 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -779,7 +779,7 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
>  				 int i, bool extra_pass, u32 stack_depth)
>  {
>  	struct bpf_insn *insn = &fp->insnsi[i];
> -	s16 branch_oc_off = insn->off;
> +	s32 branch_oc_off = insn->off;
>  	u32 dst_reg = insn->dst_reg;
>  	u32 src_reg = insn->src_reg;
>  	int last, insn_count = 1;
> -- 
> 2.43.0
> 
> 

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

[PATCH bpf 2/3] selftests/bpf: Double the size of test_loader log

[Ilya Leoshkevich]

Testing long jumps requires having >32k instructions. That many
instructions require the verifier log buffer of 2 megabytes.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/test_loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 37ffa57f28a1..b0bfcc8d4638 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -12,7 +12,7 @@
 #define str_has_pfx(str, pfx) \
 	(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
 
-#define TEST_LOADER_LOG_BUF_SZ 1048576
+#define TEST_LOADER_LOG_BUF_SZ 2097152
 
 #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
 #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
-- 
2.43.0

Re: [PATCH bpf 2/3] selftests/bpf: Double the size of test_loader log

[Yonghong Song]

On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> Testing long jumps requires having >32k instructions. That many
> instructions require the verifier log buffer of 2 megabytes.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tools/testing/selftests/bpf/test_loader.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index 37ffa57f28a1..b0bfcc8d4638 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
> @@ -12,7 +12,7 @@
>   #define str_has_pfx(str, pfx) \
>   	(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
>   
> -#define TEST_LOADER_LOG_BUF_SZ 1048576
> +#define TEST_LOADER_LOG_BUF_SZ 2097152

I think this patch is not necessary.
If the log buffer size is not enough, the kernel
verifier will wrap around and overwrite some initial states,
but all later states are still preserved. In my opinion,
there is really no need to increase the buffer size in this case,
esp. it is a verification success case.

>   
>   #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
>   #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"

Re: [PATCH bpf 2/3] selftests/bpf: Double the size of test_loader log

[Ilya Leoshkevich]

On Tue, 2024-01-02 at 16:41 -0800, Yonghong Song wrote:
> 
> On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> > Testing long jumps requires having >32k instructions. That many
> > instructions require the verifier log buffer of 2 megabytes.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   tools/testing/selftests/bpf/test_loader.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/test_loader.c
> > b/tools/testing/selftests/bpf/test_loader.c
> > index 37ffa57f28a1..b0bfcc8d4638 100644
> > --- a/tools/testing/selftests/bpf/test_loader.c
> > +++ b/tools/testing/selftests/bpf/test_loader.c
> > @@ -12,7 +12,7 @@
> >   #define str_has_pfx(str, pfx) \
> >         (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx)
> > - 1 : strlen(pfx)) == 0)
> >   
> > -#define TEST_LOADER_LOG_BUF_SZ 1048576
> > +#define TEST_LOADER_LOG_BUF_SZ 2097152
> 
> I think this patch is not necessary.
> If the log buffer size is not enough, the kernel
> verifier will wrap around and overwrite some initial states,
> but all later states are still preserved. In my opinion,
> there is really no need to increase the buffer size in this case,
> esp. it is a verification success case.

What I observed in this case was that bpf_check() still returned 
-ENOSPC and failed the prog load. IIUC you are referring to the
functionality introduced by the following commit:

commit 1216640938035e63bdbd32438e91c9bcc1fd8ee1
Author: Andrii Nakryiko <andrii@kernel.org>
Date:   Thu Apr 6 16:41:49 2023 -0700

    bpf: Switch BPF verifier log to be a rotating log by default

The commit message says, among other things:

    The only user-visible change is which portion of verifier log user
    ends up seeing *if buffer is too small*.

So if we don't increase the log size, we would still have to deal with
-ENOSPC. An alternative would be to reallocate the log buffer and try
again. But I thought that for the test code we better keep it as simple
as possible.
 
> >   #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
> >   #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"

Re: [PATCH bpf 2/3] selftests/bpf: Double the size of test_loader log

[Yonghong Song]

On 1/2/24 11:05 PM, Ilya Leoshkevich wrote:
> On Tue, 2024-01-02 at 16:41 -0800, Yonghong Song wrote:
>> On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
>>> Testing long jumps requires having >32k instructions. That many
>>> instructions require the verifier log buffer of 2 megabytes.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    tools/testing/selftests/bpf/test_loader.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/test_loader.c
>>> b/tools/testing/selftests/bpf/test_loader.c
>>> index 37ffa57f28a1..b0bfcc8d4638 100644
>>> --- a/tools/testing/selftests/bpf/test_loader.c
>>> +++ b/tools/testing/selftests/bpf/test_loader.c
>>> @@ -12,7 +12,7 @@
>>>    #define str_has_pfx(str, pfx) \
>>>          (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx)
>>> - 1 : strlen(pfx)) == 0)
>>>    
>>> -#define TEST_LOADER_LOG_BUF_SZ 1048576
>>> +#define TEST_LOADER_LOG_BUF_SZ 2097152
>> I think this patch is not necessary.
>> If the log buffer size is not enough, the kernel
>> verifier will wrap around and overwrite some initial states,
>> but all later states are still preserved. In my opinion,
>> there is really no need to increase the buffer size in this case,
>> esp. it is a verification success case.
> What I observed in this case was that bpf_check() still returned
> -ENOSPC and failed the prog load. IIUC you are referring to the
> functionality introduced by the following commit:
>
> commit 1216640938035e63bdbd32438e91c9bcc1fd8ee1
> Author: Andrii Nakryiko <andrii@kernel.org>
> Date:   Thu Apr 6 16:41:49 2023 -0700
>
>      bpf: Switch BPF verifier log to be a rotating log by default
>
> The commit message says, among other things:
>
>      The only user-visible change is which portion of verifier log user
>      ends up seeing *if buffer is too small*.
>
> So if we don't increase the log size, we would still have to deal with
> -ENOSPC. An alternative would be to reallocate the log buffer and try
> again. But I thought that for the test code we better keep it as simple
> as possible.

Okay, thanks for the explanation. I applied the patch set to
my local env and indeed, with this patch, I can see libbpf returns
an error. So as you suggested, let us increase the buffer size to
avoid extra handling in test_progs. So

Acked-by: Yonghong Song <yonghong.song@linux.dev>

>   
>>>    #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
>>>    #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"

Re: [PATCH bpf 2/3] selftests/bpf: Double the size of test_loader log

[John Fastabend]

Yonghong Song wrote:
> 
> On 1/2/24 11:05 PM, Ilya Leoshkevich wrote:
> > On Tue, 2024-01-02 at 16:41 -0800, Yonghong Song wrote:
> >> On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> >>> Testing long jumps requires having >32k instructions. That many
> >>> instructions require the verifier log buffer of 2 megabytes.
> >>>
> >>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> >>> ---
> >>>    tools/testing/selftests/bpf/test_loader.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/test_loader.c
> >>> b/tools/testing/selftests/bpf/test_loader.c
> >>> index 37ffa57f28a1..b0bfcc8d4638 100644
> >>> --- a/tools/testing/selftests/bpf/test_loader.c
> >>> +++ b/tools/testing/selftests/bpf/test_loader.c
> >>> @@ -12,7 +12,7 @@
> >>>    #define str_has_pfx(str, pfx) \
> >>>          (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx)
> >>> - 1 : strlen(pfx)) == 0)
> >>>    
> >>> -#define TEST_LOADER_LOG_BUF_SZ 1048576
> >>> +#define TEST_LOADER_LOG_BUF_SZ 2097152
> >> I think this patch is not necessary.
> >> If the log buffer size is not enough, the kernel
> >> verifier will wrap around and overwrite some initial states,
> >> but all later states are still preserved. In my opinion,
> >> there is really no need to increase the buffer size in this case,
> >> esp. it is a verification success case.
> > What I observed in this case was that bpf_check() still returned
> > -ENOSPC and failed the prog load. IIUC you are referring to the
> > functionality introduced by the following commit:
> >
> > commit 1216640938035e63bdbd32438e91c9bcc1fd8ee1
> > Author: Andrii Nakryiko <andrii@kernel.org>
> > Date:   Thu Apr 6 16:41:49 2023 -0700
> >
> >      bpf: Switch BPF verifier log to be a rotating log by default
> >
> > The commit message says, among other things:
> >
> >      The only user-visible change is which portion of verifier log user
> >      ends up seeing *if buffer is too small*.
> >
> > So if we don't increase the log size, we would still have to deal with
> > -ENOSPC. An alternative would be to reallocate the log buffer and try
> > again. But I thought that for the test code we better keep it as simple
> > as possible.
> 
> Okay, thanks for the explanation. I applied the patch set to
> my local env and indeed, with this patch, I can see libbpf returns
> an error. So as you suggested, let us increase the buffer size to
> avoid extra handling in test_progs. So
> 
> Acked-by: Yonghong Song <yonghong.song@linux.dev>

LGTM.

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

Re: [PATCH bpf 2/3] selftests/bpf: Double the size of test_loader log

[Alexei Starovoitov]

On Wed, Jan 3, 2024 at 10:15 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> Okay, thanks for the explanation. I applied the patch set to
> my local env and indeed, with this patch, I can see libbpf returns
> an error.

How did you repro this?
I've tried reverting this patch, but the test in patch 3 still passes
for me without errors.

Re: [PATCH bpf 2/3] selftests/bpf: Double the size of test_loader log

[Alexei Starovoitov]

On Thu, Jan 4, 2024 at 1:19 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jan 3, 2024 at 10:15 AM Yonghong Song <yonghong.song@linux.dev> wrote:
> >
> >
> > Okay, thanks for the explanation. I applied the patch set to
> > my local env and indeed, with this patch, I can see libbpf returns
> > an error.
>
> How did you repro this?
> I've tried reverting this patch, but the test in patch 3 still passes
> for me without errors.

Took me a long time... I was able to repro with:

diff --git a/tools/testing/selftests/bpf/progs/verifier_gotol.c
b/tools/testing/selftests/bpf/progs/verifier_gotol.c
index 05a329ee45ee..66bdb940a40b 100644
--- a/tools/testing/selftests/bpf/progs/verifier_gotol.c
+++ b/tools/testing/selftests/bpf/progs/verifier_gotol.c
@@ -36,6 +36,7 @@ l3_%=:
         \
 SEC("socket")
 __description("gotol, large_imm")
 __success __failure_unpriv __retval(40000)
+__log_level(1)
 __naked void gotol_large_imm(void)

and then I finally realized that this patch is fixing
the case when test_progs runs with -v. Like:
./test_progs -t gotol -v

I wish you mentioned this in the commit log.
Would have saved me a ton of time.

Re: [PATCH bpf 2/3] selftests/bpf: Double the size of test_loader log

[Yonghong Song]

On 1/4/24 2:33 PM, Alexei Starovoitov wrote:
> On Thu, Jan 4, 2024 at 1:19 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Wed, Jan 3, 2024 at 10:15 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>
>>> Okay, thanks for the explanation. I applied the patch set to
>>> my local env and indeed, with this patch, I can see libbpf returns
>>> an error.
>> How did you repro this?
>> I've tried reverting this patch, but the test in patch 3 still passes
>> for me without errors.
> Took me a long time... I was able to repro with:
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_gotol.c
> b/tools/testing/selftests/bpf/progs/verifier_gotol.c
> index 05a329ee45ee..66bdb940a40b 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_gotol.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_gotol.c
> @@ -36,6 +36,7 @@ l3_%=:
>           \
>   SEC("socket")
>   __description("gotol, large_imm")
>   __success __failure_unpriv __retval(40000)
> +__log_level(1)
>   __naked void gotol_large_imm(void)
>
> and then I finally realized that this patch is fixing
> the case when test_progs runs with -v. Like:
> ./test_progs -t gotol -v

Sorry for replying later. Just taking a nap...
I reproduced the issue when I tried to find out
why unpriv failed with cpuv4 as I cannot remember why.
IIRC, I am using is
   ./test_progs-cpuv4 -v -t gotol

>
> I wish you mentioned this in the commit log.
> Would have saved me a ton of time.

[PATCH bpf 3/3] selftests/bpf: Test gotol with large offsets

[Ilya Leoshkevich]

Test gotol with offsets that don't fit into a short (i.e., larger than
32k or smaller than -32k).

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 .../selftests/bpf/progs/verifier_gotol.c      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_gotol.c b/tools/testing/selftests/bpf/progs/verifier_gotol.c
index d1edbcff9a18..05a329ee45ee 100644
--- a/tools/testing/selftests/bpf/progs/verifier_gotol.c
+++ b/tools/testing/selftests/bpf/progs/verifier_gotol.c
@@ -33,6 +33,25 @@ l3_%=:							\
 	: __clobber_all);
 }
 
+SEC("socket")
+__description("gotol, large_imm")
+__success __failure_unpriv __retval(40000)
+__naked void gotol_large_imm(void)
+{
+	asm volatile ("					\
+	gotol 1f;					\
+0:							\
+	r0 = 0;						\
+	.rept 40000;					\
+	r0 += 1;					\
+	.endr;						\
+	exit;						\
+1:	gotol 0b;					\
+"	:
+	:
+	: __clobber_all);
+}
+
 #else
 
 SEC("socket")
-- 
2.43.0

Re: [PATCH bpf 3/3] selftests/bpf: Test gotol with large offsets

[Yonghong Song]

On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> Test gotol with offsets that don't fit into a short (i.e., larger than
> 32k or smaller than -32k).
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

It might be useful to explain why the test will fail
with unpriv mode (4K insn limit) just in case that
people are not aware of the reason.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   .../selftests/bpf/progs/verifier_gotol.c      | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_gotol.c b/tools/testing/selftests/bpf/progs/verifier_gotol.c
> index d1edbcff9a18..05a329ee45ee 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_gotol.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_gotol.c
> @@ -33,6 +33,25 @@ l3_%=:							\
>   	: __clobber_all);
>   }
>   
> +SEC("socket")
> +__description("gotol, large_imm")
> +__success __failure_unpriv __retval(40000)
> +__naked void gotol_large_imm(void)
> +{
> +	asm volatile ("					\
> +	gotol 1f;					\
> +0:							\
> +	r0 = 0;						\
> +	.rept 40000;					\
> +	r0 += 1;					\
> +	.endr;						\
> +	exit;						\
> +1:	gotol 0b;					\
> +"	:
> +	:
> +	: __clobber_all);
> +}
> +
>   #else
>   
>   SEC("socket")

Re: [PATCH bpf 3/3] selftests/bpf: Test gotol with large offsets

[John Fastabend]

Yonghong Song wrote:
> 
> On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> > Test gotol with offsets that don't fit into a short (i.e., larger than
> > 32k or smaller than -32k).
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> 
> It might be useful to explain why the test will fail
> with unpriv mode (4K insn limit) just in case that
> people are not aware of the reason.
> 
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> 

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

Re: [PATCH bpf 0/3] s390/bpf: Fix gotol with large offsets

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue,  2 Jan 2024 20:30:34 +0100 you wrote:
> Hi,
> 
> While looking at a pyperf180 failure on s390x (must be related to [1],
> I'm not done with the investigation yet) I noticed that I have
> unfortunately messed up the gotol implementation. Patch 1 is the fix,
> patch 2 is a small test infrastructure tweak, and patch 3 adds a
> test.
> 
> [...]

Here is the summary with links:
  - [bpf,1/3] s390/bpf: Fix gotol with large offsets
    https://git.kernel.org/bpf/bpf-next/c/ecba66cb36e3
  - [bpf,2/3] selftests/bpf: Double the size of test_loader log
    https://git.kernel.org/bpf/bpf-next/c/445aea5afda4
  - [bpf,3/3] selftests/bpf: Test gotol with large offsets
    https://git.kernel.org/bpf/bpf-next/c/63fac34669e4

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