From: Al Viro <viro@zeniv•linux.org.uk>
To: "Björn Töpel" <bjorn.topel@gmail•com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro•org>,
Netdev <netdev@vger•kernel.org>, bpf <bpf@vger•kernel.org>,
Tom Herbert <tom@herbertland•com>,
Anders Roxell <anders.roxell@gmail•com>,
linux-riscv <linux-riscv@lists•infradead.org>,
linux-arm-kernel@lists•infradead.org
Subject: Re: csum_partial() on different archs (selftest/bpf)
Date: Fri, 13 Nov 2020 14:15:42 +0000 [thread overview]
Message-ID: <20201113141542.GJ3576660@ZenIV.linux.org.uk> (raw)
In-Reply-To: <CAJ+HfNiE5Oa25QgdAdKzfk-=X45hXLKk_t+ZCiSaeFVTzgzsrw@mail.gmail.com>
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.
next prev parent reply other threads:[~2020-11-13 14:15 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-13 10:36 csum_partial() on different archs (selftest/bpf) Björn Töpel
2020-11-13 11:34 ` Eric Dumazet
2020-11-13 13:17 ` Björn Töpel
2020-11-13 12:24 ` Jean-Philippe Brucker
2020-11-13 13:22 ` Björn Töpel
2020-11-13 14:15 ` Al Viro [this message]
2020-11-13 14:32 ` Daniel Borkmann
2020-11-13 17:28 ` Al Viro
2020-11-13 12:42 ` Al Viro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201113141542.GJ3576660@ZenIV.linux.org.uk \
--to=viro@zeniv$(echo .)linux.org.uk \
--cc=anders.roxell@gmail$(echo .)com \
--cc=bjorn.topel@gmail$(echo .)com \
--cc=bpf@vger$(echo .)kernel.org \
--cc=jean-philippe@linaro$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-riscv@lists$(echo .)infradead.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=tom@herbertland$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox