[RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"

[RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"

[Paolo Pisati]

Full failure message:

...
#13/p valid read map access into a read-only array 2 FAIL retval 65507 != -29 
verification time 14 usec
stack depth 8
processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 2
peak_states 2 mark_read 1
...

this on 5.2-rc6, arm64 defconfig + CONFIG_BPF* enabled (full config here [1]).
Any idea what could be wrong?

1: http://paste.ubuntu.com/p/tXXFGCPwbp/
-- 
bye,
p.

Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"

[Yonghong Song]

On 7/1/19 4:54 AM, Paolo Pisati wrote:
> Full failure message:
> 
> ...
> #13/p valid read map access into a read-only array 2 FAIL retval 65507 != -29
> verification time 14 usec
> stack depth 8
> processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 2
> peak_states 2 mark_read 1
> ...
> 
> this on 5.2-rc6, arm64 defconfig + CONFIG_BPF* enabled (full config here [1]).
> Any idea what could be wrong?

Below is the test case.
{
         "valid read map access into a read-only array 2",
         .insns = {
         BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
         BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
         BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
         BPF_LD_MAP_FD(BPF_REG_1, 0),
         BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, 
BPF_FUNC_map_lookup_elem),
         BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),

         BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
         BPF_MOV64_IMM(BPF_REG_2, 4),
         BPF_MOV64_IMM(BPF_REG_3, 0),
         BPF_MOV64_IMM(BPF_REG_4, 0),
         BPF_MOV64_IMM(BPF_REG_5, 0),
         BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
                      BPF_FUNC_csum_diff),
         BPF_EXIT_INSN(),
         },
         .prog_type = BPF_PROG_TYPE_SCHED_CLS,
         .fixup_map_array_ro = { 3 },
         .result = ACCEPT,
         .retval = -29,
},

The issue may be with helper bpf_csum_diff().
Maybe you can check bpf_csum_diff() helper return value
to confirm and take a further look at bpf_csum_diff implementations
between x64 and amd64.

> 
> 1: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.ubuntu.com_p_tXXFGCPwbp_&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=ZFMh015yCHn-UOzwTfOnv5EUl8RHzWSgWyA7VzEAMTk&s=ZgEA0NOpPuepI2DQik8cv3bTDyF6Wx61yOB1OtnpCg0&e=
> 

Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"

[Paolo Pisati]

On Mon, Jul 01, 2019 at 09:51:25PM +0000, Yonghong Song wrote:
> 
> Below is the test case.
> {
>          "valid read map access into a read-only array 2",
>          .insns = {
>          BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>          BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>          BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>          BPF_LD_MAP_FD(BPF_REG_1, 0),
>          BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, 
> BPF_FUNC_map_lookup_elem),
>          BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
> 
>          BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
>          BPF_MOV64_IMM(BPF_REG_2, 4),
>          BPF_MOV64_IMM(BPF_REG_3, 0),
>          BPF_MOV64_IMM(BPF_REG_4, 0),
>          BPF_MOV64_IMM(BPF_REG_5, 0),
>          BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
>                       BPF_FUNC_csum_diff),
>          BPF_EXIT_INSN(),
>          },
>          .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>          .fixup_map_array_ro = { 3 },
>          .result = ACCEPT,
>          .retval = -29,
> },
> 
> The issue may be with helper bpf_csum_diff().
> Maybe you can check bpf_csum_diff() helper return value
> to confirm and take a further look at bpf_csum_diff implementations
> between x64 and amd64.

Indeed, the different result comes from csum_partial() or, more precisely,
do_csum().

x86-64 uses an asm optimized version residing in arch/x86/lib/csum-partial_64.c,
while the generic version is in lib/checksum.c.

I replaced the x86-64 csum_partial() / do_csum() code, with the one in
lib/checksum.c and by doing so i reproduced the same error on x86-64 (thus, it's
not an arch dependent issue).

I added some debugging to bpf_csum_diff(), and here are the results with different
checksum implementation code:

http://paste.debian.net/1091037/

lib/checksum.c:
...
[  206.084537] ____bpf_csum_diff from_size: 1 to_size: 0
[  206.085274] ____bpf_csum_diff from[0]: 28
[  206.085276] ____bpf_csum_diff diff[0]: 4294967267
[  206.085277] ____bpf_csum_diff diff_size: 4 seed: 0

After csum_partial() call:

[  206.086059] ____bpf_csum_diff csum: 65507 - 0xffe3

arch/x86/lib/csum-partial_64.c
...
[   40.467308] ____bpf_csum_diff from_size: 1 to_size: 0
[   40.468141] ____bpf_csum_diff from[0]: 28
[   40.468143] ____bpf_csum_diff diff[0]: 4294967267
[   40.468144] ____bpf_csum_diff diff_size: 4 seed: 0

After csum_partial() call:

[   40.468937] ____bpf_csum_diff csum: -29 - 0xffffffe3

One thing that i noticed, x86-64 csum-partial_64.c::do_csum() doesn't reduce the
calculated checksum to 16bit before returning it (unless the input value is
odd - *):

arch/x86/lib/csum-partial_64.c::do_csum()
		...
        if (unlikely(odd)) { 
                result = from32to16(result);
                result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
        }
        return result;
}

contrary to all the other do_csum() implementations (that i could understand):

lib/checksum.c::do_csum()
arch/alpha/lib/checksum.c::do_csum()
arch/parisc/lib/checksum.c::do_csum()

Apparently even ia64 does the folding (arch/ia64/lib/do_csum.S see a comment right
before .do_csum_exit:), and arch/c6x/lib/csum_64plus.S too (see
arch/c6x/lib/csum_64plus.S).

Funnily enough, if i change do_csum() for x86-64, folding the
checksum to 16 bit (following all the other implementations):

--- a/arch/x86/lib/csum-partial_64.c
+++ b/arch/x86/lib/csum-partial_64.c
@@ -112,8 +112,8 @@ static unsigned do_csum(const unsigned char *buff, unsigned
len)
        if (len & 1)
                result += *buff;
        result = add32_with_carry(result>>32, result & 0xffffffff); 
+       result = from32to16(result);
        if (unlikely(odd)) { 
-               result = from32to16(result);
                result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
        }
        return result;

then, the x86-64 result match the others: 65507 or 0xffe3.

As a last attempt, i tried running the bpf test_verifier on an armhf platform,
and i got a completely different number:

[   57.667999] ____bpf_csum_diff from_size: 1 to_size: 0
[   57.668016] ____bpf_csum_diff from[0]: 28
[   57.668028] ____bpf_csum_diff diff[0]: 4294967267
[   57.668039] ____bpf_csum_diff diff_size: 4 seed: 0

After csum_partial() call:

[   57.668052] ____bpf_csum_diff::2002 csum: 131042 - 0x0001ffe2

Not sure what to make of these number, but i have a question: whats is the
correct checksum of the memory chunk passed to csum_partial()? Is it really -29?

Because, at least 2 other implementations i tested (the arm assembly code and
the c implementation in lib/checksum.c) computes a different value, so either
there's a bug in checksum calcution (2 out of 3???), or we are interpreting the
returned value from csum_partial() somehow wrongly.

*: originally, the x86-64 did the 16bit folding, but the logic was changed to
what we have today during a big rewrite - search for:

commit 3ef076bb685a461bbaff37a1f06010fc4d7ce733
Author: Andi Kleen <ak@suse.de>
Date:   Fri Jun 13 04:27:34 2003 -0700

    [PATCH] x86-64 merge

in this historic repo:

https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
-- 
bye,
p.

Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"

[Andrii Nakryiko]

cc Andi Kleen re: refactoring, do you have any insight here?

On Wed, Jul 10, 2019 at 3:12 AM Paolo Pisati <p.pisati@gmail.com> wrote:
>
> On Mon, Jul 01, 2019 at 09:51:25PM +0000, Yonghong Song wrote:
> >
> > Below is the test case.
> > {
> >          "valid read map access into a read-only array 2",
> >          .insns = {
> >          BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> >          BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> >          BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> >          BPF_LD_MAP_FD(BPF_REG_1, 0),
> >          BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > BPF_FUNC_map_lookup_elem),
> >          BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
> >
> >          BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> >          BPF_MOV64_IMM(BPF_REG_2, 4),
> >          BPF_MOV64_IMM(BPF_REG_3, 0),
> >          BPF_MOV64_IMM(BPF_REG_4, 0),
> >          BPF_MOV64_IMM(BPF_REG_5, 0),
> >          BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> >                       BPF_FUNC_csum_diff),
> >          BPF_EXIT_INSN(),
> >          },
> >          .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> >          .fixup_map_array_ro = { 3 },
> >          .result = ACCEPT,
> >          .retval = -29,
> > },
> >
> > The issue may be with helper bpf_csum_diff().
> > Maybe you can check bpf_csum_diff() helper return value
> > to confirm and take a further look at bpf_csum_diff implementations
> > between x64 and amd64.
>
> Indeed, the different result comes from csum_partial() or, more precisely,
> do_csum().

I assume this is checksum used for ipv4 header checksum ([0]), right?
It's defined as "16-bit one's complement of the one's complement sum
of all 16-bit words", so at the end it has to be folded into 16-bit
anyways.

Reading csum_partial/csum_fold, seems like after calculation of
checksum (so-called unfolded checksum), it is supposed to be passed
into csum_fold() to convert it into 16-bit one and invert.

So maybe that's what is missing from bpf_csum_diff()? Though I've
never used it and don't even know exactly what is its purpose, so
might be (and probably am) totally wrong here (e.g., it might be that
BPF app is supposed to do that or something).

  [0] https://en.wikipedia.org/wiki/IPv4_header_checksum

>
> x86-64 uses an asm optimized version residing in arch/x86/lib/csum-partial_64.c,
> while the generic version is in lib/checksum.c.
>
> I replaced the x86-64 csum_partial() / do_csum() code, with the one in
> lib/checksum.c and by doing so i reproduced the same error on x86-64 (thus, it's
> not an arch dependent issue).
>
> I added some debugging to bpf_csum_diff(), and here are the results with different
> checksum implementation code:
>
> http://paste.debian.net/1091037/
>
> lib/checksum.c:
> ...
> [  206.084537] ____bpf_csum_diff from_size: 1 to_size: 0
> [  206.085274] ____bpf_csum_diff from[0]: 28
> [  206.085276] ____bpf_csum_diff diff[0]: 4294967267
> [  206.085277] ____bpf_csum_diff diff_size: 4 seed: 0
>
> After csum_partial() call:
>
> [  206.086059] ____bpf_csum_diff csum: 65507 - 0xffe3
>
> arch/x86/lib/csum-partial_64.c
> ...
> [   40.467308] ____bpf_csum_diff from_size: 1 to_size: 0
> [   40.468141] ____bpf_csum_diff from[0]: 28
> [   40.468143] ____bpf_csum_diff diff[0]: 4294967267
> [   40.468144] ____bpf_csum_diff diff_size: 4 seed: 0
>
> After csum_partial() call:
>
> [   40.468937] ____bpf_csum_diff csum: -29 - 0xffffffe3
>
> One thing that i noticed, x86-64 csum-partial_64.c::do_csum() doesn't reduce the
> calculated checksum to 16bit before returning it (unless the input value is
> odd - *):
>
> arch/x86/lib/csum-partial_64.c::do_csum()
>                 ...
>         if (unlikely(odd)) {
>                 result = from32to16(result);
>                 result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
>         }
>         return result;
> }
>
> contrary to all the other do_csum() implementations (that i could understand):
>
> lib/checksum.c::do_csum()
> arch/alpha/lib/checksum.c::do_csum()
> arch/parisc/lib/checksum.c::do_csum()
>
> Apparently even ia64 does the folding (arch/ia64/lib/do_csum.S see a comment right
> before .do_csum_exit:), and arch/c6x/lib/csum_64plus.S too (see
> arch/c6x/lib/csum_64plus.S).
>
> Funnily enough, if i change do_csum() for x86-64, folding the
> checksum to 16 bit (following all the other implementations):
>
> --- a/arch/x86/lib/csum-partial_64.c
> +++ b/arch/x86/lib/csum-partial_64.c
> @@ -112,8 +112,8 @@ static unsigned do_csum(const unsigned char *buff, unsigned
> len)
>         if (len & 1)
>                 result += *buff;
>         result = add32_with_carry(result>>32, result & 0xffffffff);
> +       result = from32to16(result);
>         if (unlikely(odd)) {
> -               result = from32to16(result);
>                 result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
>         }
>         return result;
>
> then, the x86-64 result match the others: 65507 or 0xffe3.
>
> As a last attempt, i tried running the bpf test_verifier on an armhf platform,
> and i got a completely different number:
>
> [   57.667999] ____bpf_csum_diff from_size: 1 to_size: 0
> [   57.668016] ____bpf_csum_diff from[0]: 28
> [   57.668028] ____bpf_csum_diff diff[0]: 4294967267
> [   57.668039] ____bpf_csum_diff diff_size: 4 seed: 0
>
> After csum_partial() call:
>
> [   57.668052] ____bpf_csum_diff::2002 csum: 131042 - 0x0001ffe2
>
> Not sure what to make of these number, but i have a question: whats is the
> correct checksum of the memory chunk passed to csum_partial()? Is it really -29?
>
> Because, at least 2 other implementations i tested (the arm assembly code and
> the c implementation in lib/checksum.c) computes a different value, so either
> there's a bug in checksum calcution (2 out of 3???), or we are interpreting the
> returned value from csum_partial() somehow wrongly.
>
> *: originally, the x86-64 did the 16bit folding, but the logic was changed to
> what we have today during a big rewrite - search for:
>
> commit 3ef076bb685a461bbaff37a1f06010fc4d7ce733
> Author: Andi Kleen <ak@suse.de>
> Date:   Fri Jun 13 04:27:34 2003 -0700
>
>     [PATCH] x86-64 merge
>
> in this historic repo:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> --
> bye,
> p.

Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"

[Andrii Nakryiko]

On Wed, Jul 10, 2019 at 3:52 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> cc Andi Kleen re: refactoring, do you have any insight here?

OK, let's try again...

>
> On Wed, Jul 10, 2019 at 3:12 AM Paolo Pisati <p.pisati@gmail.com> wrote:
> >
> > On Mon, Jul 01, 2019 at 09:51:25PM +0000, Yonghong Song wrote:
> > >
> > > Below is the test case.
> > > {
> > >          "valid read map access into a read-only array 2",
> > >          .insns = {
> > >          BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > >          BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > >          BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > >          BPF_LD_MAP_FD(BPF_REG_1, 0),
> > >          BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > > BPF_FUNC_map_lookup_elem),
> > >          BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
> > >
> > >          BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> > >          BPF_MOV64_IMM(BPF_REG_2, 4),
> > >          BPF_MOV64_IMM(BPF_REG_3, 0),
> > >          BPF_MOV64_IMM(BPF_REG_4, 0),
> > >          BPF_MOV64_IMM(BPF_REG_5, 0),
> > >          BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > >                       BPF_FUNC_csum_diff),
> > >          BPF_EXIT_INSN(),
> > >          },
> > >          .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> > >          .fixup_map_array_ro = { 3 },
> > >          .result = ACCEPT,
> > >          .retval = -29,
> > > },
> > >
> > > The issue may be with helper bpf_csum_diff().
> > > Maybe you can check bpf_csum_diff() helper return value
> > > to confirm and take a further look at bpf_csum_diff implementations
> > > between x64 and amd64.
> >
> > Indeed, the different result comes from csum_partial() or, more precisely,
> > do_csum().
>
> I assume this is checksum used for ipv4 header checksum ([0]), right?
> It's defined as "16-bit one's complement of the one's complement sum
> of all 16-bit words", so at the end it has to be folded into 16-bit
> anyways.
>
> Reading csum_partial/csum_fold, seems like after calculation of
> checksum (so-called unfolded checksum), it is supposed to be passed
> into csum_fold() to convert it into 16-bit one and invert.
>
> So maybe that's what is missing from bpf_csum_diff()? Though I've
> never used it and don't even know exactly what is its purpose, so
> might be (and probably am) totally wrong here (e.g., it might be that
> BPF app is supposed to do that or something).
>
>   [0] https://en.wikipedia.org/wiki/IPv4_header_checksum
>
> >
> > x86-64 uses an asm optimized version residing in arch/x86/lib/csum-partial_64.c,
> > while the generic version is in lib/checksum.c.
> >
> > I replaced the x86-64 csum_partial() / do_csum() code, with the one in
> > lib/checksum.c and by doing so i reproduced the same error on x86-64 (thus, it's
> > not an arch dependent issue).
> >
> > I added some debugging to bpf_csum_diff(), and here are the results with different
> > checksum implementation code:
> >
> > http://paste.debian.net/1091037/
> >
> > lib/checksum.c:
> > ...
> > [  206.084537] ____bpf_csum_diff from_size: 1 to_size: 0
> > [  206.085274] ____bpf_csum_diff from[0]: 28
> > [  206.085276] ____bpf_csum_diff diff[0]: 4294967267
> > [  206.085277] ____bpf_csum_diff diff_size: 4 seed: 0
> >
> > After csum_partial() call:
> >
> > [  206.086059] ____bpf_csum_diff csum: 65507 - 0xffe3
> >
> > arch/x86/lib/csum-partial_64.c
> > ...
> > [   40.467308] ____bpf_csum_diff from_size: 1 to_size: 0
> > [   40.468141] ____bpf_csum_diff from[0]: 28
> > [   40.468143] ____bpf_csum_diff diff[0]: 4294967267
> > [   40.468144] ____bpf_csum_diff diff_size: 4 seed: 0
> >
> > After csum_partial() call:
> >
> > [   40.468937] ____bpf_csum_diff csum: -29 - 0xffffffe3
> >
> > One thing that i noticed, x86-64 csum-partial_64.c::do_csum() doesn't reduce the
> > calculated checksum to 16bit before returning it (unless the input value is
> > odd - *):
> >
> > arch/x86/lib/csum-partial_64.c::do_csum()
> >                 ...
> >         if (unlikely(odd)) {
> >                 result = from32to16(result);
> >                 result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
> >         }
> >         return result;
> > }
> >
> > contrary to all the other do_csum() implementations (that i could understand):
> >
> > lib/checksum.c::do_csum()
> > arch/alpha/lib/checksum.c::do_csum()
> > arch/parisc/lib/checksum.c::do_csum()
> >
> > Apparently even ia64 does the folding (arch/ia64/lib/do_csum.S see a comment right
> > before .do_csum_exit:), and arch/c6x/lib/csum_64plus.S too (see
> > arch/c6x/lib/csum_64plus.S).
> >
> > Funnily enough, if i change do_csum() for x86-64, folding the
> > checksum to 16 bit (following all the other implementations):
> >
> > --- a/arch/x86/lib/csum-partial_64.c
> > +++ b/arch/x86/lib/csum-partial_64.c
> > @@ -112,8 +112,8 @@ static unsigned do_csum(const unsigned char *buff, unsigned
> > len)
> >         if (len & 1)
> >                 result += *buff;
> >         result = add32_with_carry(result>>32, result & 0xffffffff);
> > +       result = from32to16(result);
> >         if (unlikely(odd)) {
> > -               result = from32to16(result);
> >                 result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
> >         }
> >         return result;
> >
> > then, the x86-64 result match the others: 65507 or 0xffe3.
> >
> > As a last attempt, i tried running the bpf test_verifier on an armhf platform,
> > and i got a completely different number:
> >
> > [   57.667999] ____bpf_csum_diff from_size: 1 to_size: 0
> > [   57.668016] ____bpf_csum_diff from[0]: 28
> > [   57.668028] ____bpf_csum_diff diff[0]: 4294967267
> > [   57.668039] ____bpf_csum_diff diff_size: 4 seed: 0
> >
> > After csum_partial() call:
> >
> > [   57.668052] ____bpf_csum_diff::2002 csum: 131042 - 0x0001ffe2
> >
> > Not sure what to make of these number, but i have a question: whats is the
> > correct checksum of the memory chunk passed to csum_partial()? Is it really -29?
> >
> > Because, at least 2 other implementations i tested (the arm assembly code and
> > the c implementation in lib/checksum.c) computes a different value, so either
> > there's a bug in checksum calcution (2 out of 3???), or we are interpreting the
> > returned value from csum_partial() somehow wrongly.
> >
> > *: originally, the x86-64 did the 16bit folding, but the logic was changed to
> > what we have today during a big rewrite - search for:
> >
> > commit 3ef076bb685a461bbaff37a1f06010fc4d7ce733
> > Author: Andi Kleen <ak@suse.de>
> > Date:   Fri Jun 13 04:27:34 2003 -0700
> >
> >     [PATCH] x86-64 merge
> >
> > in this historic repo:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> > --
> > bye,
> > p.

Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"

[Andi Kleen]

> > Reading csum_partial/csum_fold, seems like after calculation of
> > checksum (so-called unfolded checksum), it is supposed to be passed
> > into csum_fold() to convert it into 16-bit one and invert.

Yes, you always need to fold at the end.

The low level code does fold sometimes, but not always.

-Andi

[PATCH 0/2] [RESEND] Fold checksum at the end of bpf_csum_diff and fix

[Paolo Pisati]

From: Paolo Pisati <paolo.pisati@canonical.com>

After applying patch 0001, all checksum implementations i could test (x86-64, arm64 and
arm), now agree on the return value.

Patch 0002 fix the expected return value for test #13: i did the calculation manually,
and it correspond.

Unfortunately, after applying patch 0001, other test cases now fail in
test_verifier:

$ sudo ./tools/testing/selftests/bpf/test_verifier
...
#417/p helper access to variable memory: size = 0 allowed on NULL (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 
#419/p helper access to variable memory: size = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 
#423/p helper access to variable memory: size possible = 0 allowed on != NULL packet pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 
...
Summary: 1500 PASSED, 0 SKIPPED, 3 FAILED

And there are probably other fallouts in other selftests - someone familiar
should take a look before applying these patches.

Paolo Pisati (2):
  bpf: bpf_csum_diff: fold the checksum before returning the
    value
  bpf, selftest: fix checksum value for test #13

 net/core/filter.c                                   | 2 +-
 tools/testing/selftests/bpf/verifier/array_access.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] bpf: bpf_csum_diff: fold the checksum before returning the value

[Paolo Pisati]

From: Paolo Pisati <paolo.pisati@canonical.com>

With this change, bpf_csum_diff behave homogeneously among different
checksum calculation code / csum_partial() (tested on x86-64, arm64 and
arm).

Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
---
 net/core/filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index f615e42cf4ef..8db7f58f1ea1 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1990,7 +1990,7 @@ BPF_CALL_5(bpf_csum_diff, __be32 *, from, u32, from_size,
 	for (i = 0; i <   to_size / sizeof(__be32); i++, j++)
 		sp->diff[j] = to[i];
 
-	return csum_partial(sp->diff, diff_size, seed);
+	return csum_fold(csum_partial(sp->diff, diff_size, seed));
 }
 
 static const struct bpf_func_proto bpf_csum_diff_proto = {
-- 
2.17.1

[PATCH 2/2] bpf, selftest: fix checksum value for test #13

[Paolo Pisati]

From: Paolo Pisati <paolo.pisati@canonical.com>

Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
---
 tools/testing/selftests/bpf/verifier/array_access.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c
index bcb83196e459..4698f560d756 100644
--- a/tools/testing/selftests/bpf/verifier/array_access.c
+++ b/tools/testing/selftests/bpf/verifier/array_access.c
@@ -255,7 +255,7 @@
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	.fixup_map_array_ro = { 3 },
 	.result = ACCEPT,
-	.retval = -29,
+	.retval = 28,
 },
 {
 	"invalid write map access into a read-only array 1",
-- 
2.17.1

Re: [PATCH 2/2] bpf, selftest: fix checksum value for test #13

[Andrii Nakryiko]

On Thu, Jul 11, 2019 at 2:41 AM Paolo Pisati <p.pisati@gmail.com> wrote:
>
> From: Paolo Pisati <paolo.pisati@canonical.com>
>

Please include description, in addition to subject.

Also, when submitting patches, please add bpf or bpf-next (e.g.,
[PATCH bpf 2/2] to indicate which tree it's supposed to go into). For
this one it's probably bpf.


> Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
> ---
>  tools/testing/selftests/bpf/verifier/array_access.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c
> index bcb83196e459..4698f560d756 100644
> --- a/tools/testing/selftests/bpf/verifier/array_access.c
> +++ b/tools/testing/selftests/bpf/verifier/array_access.c
> @@ -255,7 +255,7 @@
>         .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>         .fixup_map_array_ro = { 3 },
>         .result = ACCEPT,
> -       .retval = -29,
> +       .retval = 28,
>  },
>  {
>         "invalid write map access into a read-only array 1",
> --
> 2.17.1
>

[PATCH 0/2] Fold checksum at the end of bpf_csum_diff and fix

[Paolo Pisati]

From: Paolo Pisati <paolo.pisati@canonical.com>

After applying patch 0001, all checksum implementations i could test (x86-64, arm64 and
arm), now agree on the return value.

Patch 0002 fix the expected return value for test #13: i did the calculation manually,
and it correspond.

Unfortunately, after applying patch 0001, other test cases now fail in
test_verifier:

$ sudo ./tools/testing/selftests/bpf/test_verifier
...
#417/p helper access to variable memory: size = 0 allowed on NULL (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 
#419/p helper access to variable memory: size = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 
#423/p helper access to variable memory: size possible = 0 allowed on != NULL packet pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 
...
Summary: 1500 PASSED, 0 SKIPPED, 3 FAILED

And there are probably other fallouts in other selftests - someone familiar
should take a look before applying these patches.

Paolo Pisati (2):
  bpf: bpf_csum_diff: fold the checksum before returning the
    value
  bpf, selftest: fix checksum value for test #13

 net/core/filter.c                                   | 2 +-
 tools/testing/selftests/bpf/verifier/array_access.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] bpf: bpf_csum_diff: fold the checksum before returning the value

[Paolo Pisati]

From: Paolo Pisati <paolo.pisati@canonical.com>

With this change, bpf_csum_diff behave homogeneously among different
checksum calculation code / csum_partial() (tested on x86-64, arm64 and
arm).

Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
---
 net/core/filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index f615e42cf4ef..8db7f58f1ea1 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1990,7 +1990,7 @@ BPF_CALL_5(bpf_csum_diff, __be32 *, from, u32, from_size,
 	for (i = 0; i <   to_size / sizeof(__be32); i++, j++)
 		sp->diff[j] = to[i];
 
-	return csum_partial(sp->diff, diff_size, seed);
+	return csum_fold(csum_partial(sp->diff, diff_size, seed));
 }
 
 static const struct bpf_func_proto bpf_csum_diff_proto = {
-- 
2.17.1