csum_partial() on different archs (selftest/bpf)

csum_partial() on different archs (selftest/bpf)

[Björn Töpel]

I was running the selftest/bpf on riscv, and had a closer look at one
of the failing cases:

  #14/p valid read map access into a read-only array 2 FAIL retval
65507 != -29 (run 1/1)

The test does a csum_partial() call via a BPF helper. riscv uses the
generic implementation. arm64 uses the generic csum_partial() and fail
in the same way [1]. arm (32-bit) has a arch specfic implementation,
and fail in another way (FAIL retval 131042 != -29) [2].

I mimicked the test case in a userland program, comparing the generic
csum_partial() to the x86 implementation [3], and the generic and x86
implementation does yield a different result.

x86     :    -29 : 0xffffffe3
generic :  65507 : 0x0000ffe3
arm     : 131042 : 0x0001ffe2

Who is correct? :-) It would be nice to get rid of this failed case...


Thanks,
Björn


[1] https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20201112/testrun/3430401/suite/kselftest/test/bpf.test_verifier/log
[2] https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v5.10-rc3-207-g585e5b17b92d/testrun/3432361/suite/kselftest/test/bpf.test_verifier/log
[3] https://gist.github.com/bjoto/dc22d593aa3ac63c2c90632de5ed82e0

Re: csum_partial() on different archs (selftest/bpf)

[Eric Dumazet]

On 11/13/20 11:36 AM, Björn Töpel wrote:
> I was running the selftest/bpf on riscv, and had a closer look at one
> of the failing cases:
> 
>   #14/p valid read map access into a read-only array 2 FAIL retval
> 65507 != -29 (run 1/1)
> 
> The test does a csum_partial() call via a BPF helper. riscv uses the
> generic implementation. arm64 uses the generic csum_partial() and fail
> in the same way [1]. arm (32-bit) has a arch specfic implementation,
> and fail in another way (FAIL retval 131042 != -29) [2].
> 
> I mimicked the test case in a userland program, comparing the generic
> csum_partial() to the x86 implementation [3], and the generic and x86
> implementation does yield a different result.
> 
> x86     :    -29 : 0xffffffe3
> generic :  65507 : 0x0000ffe3
> arm     : 131042 : 0x0001ffe2
> 
> Who is correct? :-) It would be nice to get rid of this failed case...
> 

There are all the same value :), they all fold to u16  0xFFE3

Maybe the test needs a fix, there is a missing folding.

> 
> Thanks,
> Björn
> 
> 
> [1] https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20201112/testrun/3430401/suite/kselftest/test/bpf.test_verifier/log
> [2] https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v5.10-rc3-207-g585e5b17b92d/testrun/3432361/suite/kselftest/test/bpf.test_verifier/log
> [3] https://gist.github.com/bjoto/dc22d593aa3ac63c2c90632de5ed82e0
> 

Re: csum_partial() on different archs (selftest/bpf)

[Björn Töpel]

On Fri, 13 Nov 2020 at 12:34, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 11/13/20 11:36 AM, Björn Töpel wrote:
> > I was running the selftest/bpf on riscv, and had a closer look at one
> > of the failing cases:
> >
> >   #14/p valid read map access into a read-only array 2 FAIL retval
> > 65507 != -29 (run 1/1)
> >
> > The test does a csum_partial() call via a BPF helper. riscv uses the
> > generic implementation. arm64 uses the generic csum_partial() and fail
> > in the same way [1]. arm (32-bit) has a arch specfic implementation,
> > and fail in another way (FAIL retval 131042 != -29) [2].
> >
> > I mimicked the test case in a userland program, comparing the generic
> > csum_partial() to the x86 implementation [3], and the generic and x86
> > implementation does yield a different result.
> >
> > x86     :    -29 : 0xffffffe3
> > generic :  65507 : 0x0000ffe3
> > arm     : 131042 : 0x0001ffe2
> >
> > Who is correct? :-) It would be nice to get rid of this failed case...
> >
>
> There are all the same value :), they all fold to u16  0xFFE3
>
> Maybe the test needs a fix, there is a missing folding.
>

Ah, makes sense. Thank you!

Björn

Re: csum_partial() on different archs (selftest/bpf)

[Jean-Philippe Brucker]

Hi,

On Fri, Nov 13, 2020 at 11:36:08AM +0100, Björn Töpel wrote:
> I was running the selftest/bpf on riscv, and had a closer look at one
> of the failing cases:
> 
>   #14/p valid read map access into a read-only array 2 FAIL retval
> 65507 != -29 (run 1/1)
> 
> The test does a csum_partial() call via a BPF helper. riscv uses the
> generic implementation. arm64 uses the generic csum_partial() and fail
> in the same way [1].

It's worse than that, because arm64, parisc, alpha and others implement
do_csum(), called by the generic csum_partial(), and those all return a
16-bit value.

It would be good to firstly formalize the size of the value returned by
the bpf_csum_diff() helper, because it's not clear from the doc (and the
helper returns a s64).

Then homogenizing the csum_partial() implementations is difficult. One way
forward, without having to immediately rewrite all arch-specific
implementations, would be to replace csum_partial() and do_csum() with
csum_partial_32(), csum_partial_16(), do_csum_32() and do_csum_16(). That
way we can use a generic implementation of the 32-bit variant if the
arch-specific implementation is 16-bit.

Thanks,
Jean

> arm (32-bit) has a arch specfic implementation,
> and fail in another way (FAIL retval 131042 != -29) [2].
> 
> I mimicked the test case in a userland program, comparing the generic
> csum_partial() to the x86 implementation [3], and the generic and x86
> implementation does yield a different result.
> 
> x86     :    -29 : 0xffffffe3
> generic :  65507 : 0x0000ffe3
> arm     : 131042 : 0x0001ffe2
> 
> Who is correct? :-) It would be nice to get rid of this failed case...
> 
> 
> Thanks,
> Björn
> 
> 
> [1] https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20201112/testrun/3430401/suite/kselftest/test/bpf.test_verifier/log
> [2] https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v5.10-rc3-207-g585e5b17b92d/testrun/3432361/suite/kselftest/test/bpf.test_verifier/log
> [3] https://gist.github.com/bjoto/dc22d593aa3ac63c2c90632de5ed82e0

Re: csum_partial() on different archs (selftest/bpf)

[Björn Töpel]

On Fri, 13 Nov 2020 at 13:25, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> Hi,
>
> On Fri, Nov 13, 2020 at 11:36:08AM +0100, Björn Töpel wrote:
> > I was running the selftest/bpf on riscv, and had a closer look at one
> > of the failing cases:
> >
> >   #14/p valid read map access into a read-only array 2 FAIL retval
> > 65507 != -29 (run 1/1)
> >
> > The test does a csum_partial() call via a BPF helper. riscv uses the
> > generic implementation. arm64 uses the generic csum_partial() and fail
> > in the same way [1].
>
> It's worse than that, because arm64, parisc, alpha and others implement
> do_csum(), called by the generic csum_partial(), and those all return a
> 16-bit value.
>
> It would be good to firstly formalize the size of the value returned by
> the bpf_csum_diff() helper, because it's not clear from the doc (and the
> helper returns a s64).
>
> Then homogenizing the csum_partial() implementations is difficult. One way
> forward, without having to immediately rewrite all arch-specific
> implementations, would be to replace csum_partial() and do_csum() with
> csum_partial_32(), csum_partial_16(), do_csum_32() and do_csum_16(). That
> way we can use a generic implementation of the 32-bit variant if the
> arch-specific implementation is 16-bit.
>

Folding Al's input to this reply.

I think the bpf_csum_diff() is supposed to be used in combination with
another helper(s) (e.g. bpf_l4_csum_replace) so I'd guess the returned
__wsum should be seen as an opaque value, not something BPF userland
can rely on.

So, for this specific test case, it's probably best to just update the
test case (as Eric suggested).


Cheers, and thanks for the input!
Björn

Re: csum_partial() on different archs (selftest/bpf)

[Al Viro]

On Fri, Nov 13, 2020 at 02:22:16PM +0100, Björn Töpel wrote:

> Folding Al's input to this reply.
> 
> I think the bpf_csum_diff() is supposed to be used in combination with
> another helper(s) (e.g. bpf_l4_csum_replace) so I'd guess the returned
> __wsum should be seen as an opaque value, not something BPF userland
> can rely on.

Why not reduce the sucker modulo 0xffff before returning it?  Incidentally,
implementation is bloody awful:

        /* This is quite flexible, some examples:
         *
         * from_size == 0, to_size > 0,  seed := csum --> pushing data
         * from_size > 0,  to_size == 0, seed := csum --> pulling data
         * from_size > 0,  to_size > 0,  seed := 0    --> diffing data
         *
         * Even for diffing, from_size and to_size don't need to be equal.
         */
        if (unlikely(((from_size | to_size) & (sizeof(__be32) - 1)) ||
                     diff_size > sizeof(sp->diff)))
                return -EINVAL;

        for (i = 0; i < from_size / sizeof(__be32); i++, j++)
                sp->diff[j] = ~from[i];
        for (i = 0; i <   to_size / sizeof(__be32); i++, j++)
                sp->diff[j] = to[i];

        return csum_partial(sp->diff, diff_size, seed);

What the hell is this (copying, scratchpad, etc.) for?  First of all,
_if_ you want to use csum_partial() at all (and I'm not at all sure
that it won't be cheaper to just go over two arrays, doing csum_add()
and csum_sub() resp. - depends upon the realistic sizes), you don't
need to copy anything.  Find the sum of from, find the sum of to and
then subtract (csum_sub()) the old sum from the seed and and add the
new one...

And I would strongly recommend to change the calling conventions of that
thing - make it return __sum16.  And take __sum16 as well...

Again, exposing __wsum to anything that looks like a stable ABI is
a mistake - it's an internal detail that can be easily abused,
causing unpleasant compat problems.

Re: csum_partial() on different archs (selftest/bpf)

[Daniel Borkmann]

On 11/13/20 3:15 PM, Al Viro wrote:
> On Fri, Nov 13, 2020 at 02:22:16PM +0100, Björn Töpel wrote:
> 
>> Folding Al's input to this reply.
>>
>> I think the bpf_csum_diff() is supposed to be used in combination with
>> another helper(s) (e.g. bpf_l4_csum_replace) so I'd guess the returned
>> __wsum should be seen as an opaque value, not something BPF userland
>> can rely on.
> 
> Why not reduce the sucker modulo 0xffff before returning it?  Incidentally,
> implementation is bloody awful:
> 
>          /* This is quite flexible, some examples:
>           *
>           * from_size == 0, to_size > 0,  seed := csum --> pushing data
>           * from_size > 0,  to_size == 0, seed := csum --> pulling data
>           * from_size > 0,  to_size > 0,  seed := 0    --> diffing data
>           *
>           * Even for diffing, from_size and to_size don't need to be equal.
>           */
>          if (unlikely(((from_size | to_size) & (sizeof(__be32) - 1)) ||
>                       diff_size > sizeof(sp->diff)))
>                  return -EINVAL;
> 
>          for (i = 0; i < from_size / sizeof(__be32); i++, j++)
>                  sp->diff[j] = ~from[i];
>          for (i = 0; i <   to_size / sizeof(__be32); i++, j++)
>                  sp->diff[j] = to[i];
> 
>          return csum_partial(sp->diff, diff_size, seed);
> 
> What the hell is this (copying, scratchpad, etc.) for?  First of all,
> _if_ you want to use csum_partial() at all (and I'm not at all sure
> that it won't be cheaper to just go over two arrays, doing csum_add()
> and csum_sub() resp. - depends upon the realistic sizes), you don't
> need to copy anything.  Find the sum of from, find the sum of to and
> then subtract (csum_sub()) the old sum from the seed and and add the
> new one...
> 
> And I would strongly recommend to change the calling conventions of that
> thing - make it return __sum16.  And take __sum16 as well...
> 
> Again, exposing __wsum to anything that looks like a stable ABI is
> a mistake - it's an internal detail that can be easily abused,
> causing unpleasant compat problems.

I'll take a look at both, removing the copying and also wrt not breaking
existing users for cascading the helper when fixing.

Thanks,
Daniel

Re: csum_partial() on different archs (selftest/bpf)

[Al Viro]

On Fri, Nov 13, 2020 at 03:32:22PM +0100, Daniel Borkmann wrote:

> > And I would strongly recommend to change the calling conventions of that
> > thing - make it return __sum16.  And take __sum16 as well...
> > 
> > Again, exposing __wsum to anything that looks like a stable ABI is
> > a mistake - it's an internal detail that can be easily abused,
> > causing unpleasant compat problems.
> 
> I'll take a look at both, removing the copying and also wrt not breaking
> existing users for cascading the helper when fixing.

FWIW, see below the patch that sits in the leftovers queue (didn't make it into
work.csum_and_copy, missed the window, didn't get around to dealing with that
for -next this cycle yet); it does not fold the result, but deals with the rest
of that fun.  I would still suggest at least folding the result; something like
	return csum_fold(csum_sub(csum_partial(from, from_size, 0),
			          csum_partial(to, to_size, seed)),
instead of what this patch does, to guarantee a normalized return value.
Note that the order of csum_sub() arguments here is inverted compared to the
patch below  - csum_fold() returns reduced *complement* of its argument, so we
want to give it SUM(from) - SUM(to) - seed, not seed - SUM(from) + SUM(to).
And it's probably a separate followup (adding normalization, that is).

commit 1dd99d9664ec36e9068afb3ca0017c0a43ee420f
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Wed Jul 8 00:07:11 2020 -0400

    bpf_csum_diff(): don't bother with scratchpads
    
    Just use call csum_partial() on to and from and use csum_sub().
    No need to bother with copying, inverting, percpu scratchpads,
    etc.
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/net/core/filter.c b/net/core/filter.c
index 7124f0fe6974..3e21327b9964 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1635,15 +1635,6 @@ void sk_reuseport_prog_free(struct bpf_prog *prog)
 		bpf_prog_destroy(prog);
 }
 
-struct bpf_scratchpad {
-	union {
-		__be32 diff[MAX_BPF_STACK / sizeof(__be32)];
-		u8     buff[MAX_BPF_STACK];
-	};
-};
-
-static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);
-
 static inline int __bpf_try_make_writable(struct sk_buff *skb,
 					  unsigned int write_len)
 {
@@ -1987,10 +1978,6 @@ static const struct bpf_func_proto bpf_l4_csum_replace_proto = {
 BPF_CALL_5(bpf_csum_diff, __be32 *, from, u32, from_size,
 	   __be32 *, to, u32, to_size, __wsum, seed)
 {
-	struct bpf_scratchpad *sp = this_cpu_ptr(&bpf_sp);
-	u32 diff_size = from_size + to_size;
-	int i, j = 0;
-
 	/* This is quite flexible, some examples:
 	 *
 	 * from_size == 0, to_size > 0,  seed := csum --> pushing data
@@ -1999,16 +1986,11 @@ BPF_CALL_5(bpf_csum_diff, __be32 *, from, u32, from_size,
 	 *
 	 * Even for diffing, from_size and to_size don't need to be equal.
 	 */
-	if (unlikely(((from_size | to_size) & (sizeof(__be32) - 1)) ||
-		     diff_size > sizeof(sp->diff)))
+	if (unlikely((from_size | to_size) & (sizeof(__be32) - 1)))
 		return -EINVAL;
 
-	for (i = 0; i < from_size / sizeof(__be32); i++, j++)
-		sp->diff[j] = ~from[i];
-	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_sub(csum_partial(to, to_size, seed),
+			csum_partial(from, from_size, 0));
 }
 
 static const struct bpf_func_proto bpf_csum_diff_proto = {

Re: csum_partial() on different archs (selftest/bpf)

[Al Viro]

On Fri, Nov 13, 2020 at 11:36:08AM +0100, Björn Töpel wrote:
> I was running the selftest/bpf on riscv, and had a closer look at one
> of the failing cases:
> 
>   #14/p valid read map access into a read-only array 2 FAIL retval
> 65507 != -29 (run 1/1)
> 
> The test does a csum_partial() call via a BPF helper. riscv uses the
> generic implementation. arm64 uses the generic csum_partial() and fail
> in the same way [1]. arm (32-bit) has a arch specfic implementation,
> and fail in another way (FAIL retval 131042 != -29) [2].
> 
> I mimicked the test case in a userland program, comparing the generic
> csum_partial() to the x86 implementation [3], and the generic and x86
> implementation does yield a different result.
> 
> x86     :    -29 : 0xffffffe3
> generic :  65507 : 0x0000ffe3
> arm     : 131042 : 0x0001ffe2
> 
> Who is correct? :-) It would be nice to get rid of this failed case...

Don't expose unfolded csums to *anything* that might care about the
specific bit pattern.   All you are guaranteed is the value mod 0xffff.
Full 32bit value is not just arch-specific - it can change from moving
the area you are giving it by two bytes.  Yes, really.

It's *NOT* suitable for passig to userland.  Or for sending over the
wire.  Or for storing in filesystem metadata (as reiserfs xattrs have
done).

__wsum is purely internal thing; BPF has no business sticking its
fingers there, let alone exposing it as part of any kind of stable ABI.