csum_partial() on different archs (selftest/bpf)

[Björn Töpel]

On Fri, 13 Nov 2020 at 13:25, Jean-Philippe Brucker
<jean-philippe at 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