From: Brendan Jackman <jackmanb@google.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Yonghong Song <yhs@fb.com>,
Daniel Borkmann <daniel@iogearbox.net>,
KP Singh <kpsingh@chromium.org>,
Florent Revest <revest@chromium.org>,
linux-kernel@vger.kernel.org, Jann Horn <jannh@google.com>
Subject: Re: [PATCH bpf-next v4 04/11] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
Date: Tue, 8 Dec 2020 09:26:50 +0000 [thread overview]
Message-ID: <X89G2kItO2o60+A6@google.com> (raw)
In-Reply-To: <5fcea525c4971_5a96208bd@john-XPS-13-9370.notmuch>
Hi John, thanks a lot for the reviews!
On Mon, Dec 07, 2020 at 01:56:53PM -0800, John Fastabend wrote:
> Brendan Jackman wrote:
> > A subsequent patch will add additional atomic operations. These new
> > operations will use the same opcode field as the existing XADD, with
> > the immediate discriminating different operations.
> >
> > In preparation, rename the instruction mode BPF_ATOMIC and start
> > calling the zero immediate BPF_ADD.
> >
> > This is possible (doesn't break existing valid BPF progs) because the
> > immediate field is currently reserved MBZ and BPF_ADD is zero.
> >
> > All uses are removed from the tree but the BPF_XADD definition is
> > kept around to avoid breaking builds for people including kernel
> > headers.
> >
> > Signed-off-by: Brendan Jackman <jackmanb@google.com>
> > ---
> > Documentation/networking/filter.rst | 30 ++++++++-----
> > arch/arm/net/bpf_jit_32.c | 7 ++-
> > arch/arm64/net/bpf_jit_comp.c | 16 +++++--
> > arch/mips/net/ebpf_jit.c | 11 +++--
> > arch/powerpc/net/bpf_jit_comp64.c | 25 ++++++++---
> > arch/riscv/net/bpf_jit_comp32.c | 20 +++++++--
> > arch/riscv/net/bpf_jit_comp64.c | 16 +++++--
> > arch/s390/net/bpf_jit_comp.c | 27 ++++++-----
> > arch/sparc/net/bpf_jit_comp_64.c | 17 +++++--
> > arch/x86/net/bpf_jit_comp.c | 45 ++++++++++++++-----
> > arch/x86/net/bpf_jit_comp32.c | 6 +--
> > drivers/net/ethernet/netronome/nfp/bpf/jit.c | 14 ++++--
> > drivers/net/ethernet/netronome/nfp/bpf/main.h | 4 +-
> > .../net/ethernet/netronome/nfp/bpf/verifier.c | 15 ++++---
> > include/linux/filter.h | 29 ++++++++++--
> > include/uapi/linux/bpf.h | 5 ++-
> > kernel/bpf/core.c | 31 +++++++++----
> > kernel/bpf/disasm.c | 6 ++-
> > kernel/bpf/verifier.c | 24 +++++-----
> > lib/test_bpf.c | 14 +++---
> > samples/bpf/bpf_insn.h | 4 +-
> > samples/bpf/cookie_uid_helper_example.c | 6 +--
> > samples/bpf/sock_example.c | 2 +-
> > samples/bpf/test_cgrp2_attach.c | 5 ++-
> > tools/include/linux/filter.h | 28 ++++++++++--
> > tools/include/uapi/linux/bpf.h | 5 ++-
> > .../bpf/prog_tests/cgroup_attach_multi.c | 4 +-
> > .../selftests/bpf/test_cgroup_storage.c | 2 +-
> > tools/testing/selftests/bpf/verifier/ctx.c | 7 ++-
> > .../bpf/verifier/direct_packet_access.c | 4 +-
> > .../testing/selftests/bpf/verifier/leak_ptr.c | 10 ++---
> > .../selftests/bpf/verifier/meta_access.c | 4 +-
> > tools/testing/selftests/bpf/verifier/unpriv.c | 3 +-
> > .../bpf/verifier/value_illegal_alu.c | 2 +-
> > tools/testing/selftests/bpf/verifier/xadd.c | 18 ++++----
> > 35 files changed, 317 insertions(+), 149 deletions(-)
> >
>
> [...]
>
> > +++ a/arch/mips/net/ebpf_jit.c
>
> [...]
>
> > - if (BPF_MODE(insn->code) == BPF_XADD) {
> > + if (BPF_MODE(insn->code) == BPF_ATOMIC) {
> > + if (insn->imm != BPF_ADD) {
> > + pr_err("ATOMIC OP %02x NOT HANDLED\n", insn->imm);
> > + return -EINVAL;
> > + }
> > +
> > /*
> [...]
> > +++ b/arch/powerpc/net/bpf_jit_comp64.c
>
> > - case BPF_STX | BPF_XADD | BPF_W:
> > + case BPF_STX | BPF_ATOMIC | BPF_W:
> > + if (insn->imm != BPF_ADD) {
> > + pr_err_ratelimited(
> > + "eBPF filter atomic op code %02x (@%d) unsupported\n",
> > + code, i);
> > + return -ENOTSUPP;
> > + }
> [...]
> > @@ -699,8 +707,15 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
> > - case BPF_STX | BPF_XADD | BPF_DW:
> > + case BPF_STX | BPF_ATOMIC | BPF_DW:
> > + if (insn->imm != BPF_ADD) {
> > + pr_err_ratelimited(
> > + "eBPF filter atomic op code %02x (@%d) unsupported\n",
> > + code, i);
> > + return -ENOTSUPP;
> > + }
> [...]
> > + case BPF_STX | BPF_ATOMIC | BPF_W:
> > + if (insn->imm != BPF_ADD) {
> > + pr_info_once(
> > + "bpf-jit: not supported: atomic operation %02x ***\n",
> > + insn->imm);
> > + return -EFAULT;
> > + }
> [...]
> > + case BPF_STX | BPF_ATOMIC | BPF_W:
> > + case BPF_STX | BPF_ATOMIC | BPF_DW:
> > + if (insn->imm != BPF_ADD) {
> > + pr_err("bpf-jit: not supported: atomic operation %02x ***\n",
> > + insn->imm);
> > + return -EINVAL;
> > + }
>
> Can we standardize the error across jits and the error return code? It seems
> odd that we use pr_err, pr_info_once, pr_err_ratelimited and then return
> ENOTSUPP, EFAULT or EINVAL.
That would be a noble cause but I don't think it makes sense in this
patchset: they are already inconsistent, so here I've gone for intra-JIT
consistency over inter-JIT consistency.
I think it would be more annoying, for example, if the s390 JIT returned
-EOPNOTSUPP for a bad atomic but -1 for other unsupported ops, than it
is already that the s390 JIT returns -1 where the MIPS returns -EINVAL.
> granted the error codes might not propagate all the way out at the moment but
> still shouldn't hurt.
>
> > diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> > index 0a4182792876..f973e2ead197 100644
> > --- a/arch/s390/net/bpf_jit_comp.c
> > +++ b/arch/s390/net/bpf_jit_comp.c
> > @@ -1205,18 +1205,23 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
>
> For example this will return -1 regardless of error from insn->imm != BPF_ADD.
> [...]
> > + case BPF_STX | BPF_ATOMIC | BPF_DW:
> > + case BPF_STX | BPF_ATOMIC | BPF_W:
> > + if (insn->imm != BPF_ADD) {
> > + pr_err("Unknown atomic operation %02x\n", insn->imm);
> > + return -1;
> > + }
> > +
> [...]
>
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -259,15 +259,38 @@ static inline bool insn_is_zext(const struct bpf_insn *insn)
> > .off = OFF, \
> > .imm = 0 })
> >
> > -/* Atomic memory add, *(uint *)(dst_reg + off16) += src_reg */
> > +
> > +/*
> > + * Atomic operations:
> > + *
> > + * BPF_ADD *(uint *) (dst_reg + off16) += src_reg
> > + */
> > +
> > +#define BPF_ATOMIC64(OP, DST, SRC, OFF) \
> > + ((struct bpf_insn) { \
> > + .code = BPF_STX | BPF_DW | BPF_ATOMIC, \
> > + .dst_reg = DST, \
> > + .src_reg = SRC, \
> > + .off = OFF, \
> > + .imm = OP })
> > +
> > +#define BPF_ATOMIC32(OP, DST, SRC, OFF) \
> > + ((struct bpf_insn) { \
> > + .code = BPF_STX | BPF_W | BPF_ATOMIC, \
> > + .dst_reg = DST, \
> > + .src_reg = SRC, \
> > + .off = OFF, \
> > + .imm = OP })
> > +
> > +/* Legacy equivalent of BPF_ATOMIC{64,32}(BPF_ADD, ...) */
>
> Not sure I care too much. Does seem more natural to follow
> below pattern and use,
>
> BPF_ATOMIC(OP, SIZE, DST, SRC, OFF)
>
> >
> > #define BPF_STX_XADD(SIZE, DST, SRC, OFF) \
> > ((struct bpf_insn) { \
> > - .code = BPF_STX | BPF_SIZE(SIZE) | BPF_XADD, \
> > + .code = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC, \
> > .dst_reg = DST, \
> > .src_reg = SRC, \
> > .off = OFF, \
> > - .imm = 0 })
> > + .imm = BPF_ADD })
> >
> > /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
> >
>
> [...]
>
> Otherwise LGTM, I'll try to get the remaining patches reviewed tonight
> I need to jump onto something else this afternoon. Thanks!
next prev parent reply other threads:[~2020-12-08 9:27 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-07 16:07 [PATCH bpf-next v4 00/11] Atomics for eBPF Brendan Jackman
2020-12-07 16:07 ` [PATCH bpf-next v4 01/11] bpf: x86: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
2020-12-07 21:04 ` John Fastabend
2020-12-07 16:07 ` [PATCH bpf-next v4 02/11] bpf: x86: Factor out emission of REX byte Brendan Jackman
2020-12-07 21:07 ` John Fastabend
2020-12-07 16:07 ` [PATCH bpf-next v4 03/11] bpf: x86: Factor out a lookup table for some ALU opcodes Brendan Jackman
2020-12-07 21:08 ` John Fastabend
2020-12-07 16:07 ` [PATCH bpf-next v4 04/11] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
2020-12-07 21:56 ` John Fastabend
2020-12-08 9:26 ` Brendan Jackman [this message]
2020-12-09 5:40 ` John Fastabend
2020-12-07 16:07 ` [PATCH bpf-next v4 05/11] bpf: Move BPF_STX reserved field check into BPF_STX verifier code Brendan Jackman
2020-12-08 1:35 ` Yonghong Song
2020-12-08 5:13 ` John Fastabend
2020-12-07 16:07 ` [PATCH bpf-next v4 06/11] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
2020-12-08 1:41 ` Yonghong Song
2020-12-08 9:31 ` Brendan Jackman
2020-12-08 5:31 ` John Fastabend
2020-12-08 9:59 ` Brendan Jackman
2020-12-07 16:07 ` [PATCH bpf-next v4 07/11] bpf: Add instructions for atomic_[cmp]xchg Brendan Jackman
2020-12-08 1:44 ` Yonghong Song
2020-12-08 6:37 ` John Fastabend
2020-12-14 15:39 ` Brendan Jackman
2020-12-08 6:42 ` John Fastabend
2020-12-07 16:07 ` [PATCH bpf-next v4 08/11] bpf: Pull out a macro for interpreting atomic ALU operations Brendan Jackman
2020-12-07 16:07 ` [PATCH bpf-next v4 09/11] bpf: Add bitwise atomic instructions Brendan Jackman
2020-12-08 1:47 ` Yonghong Song
2020-12-10 0:22 ` kernel test robot
2020-12-10 0:22 ` kernel test robot
2020-12-07 16:07 ` [PATCH bpf-next v4 10/11] bpf: Add tests for new BPF atomic operations Brendan Jackman
2020-12-08 3:18 ` Yonghong Song
2020-12-08 12:41 ` Brendan Jackman
2020-12-08 16:38 ` Yonghong Song
2020-12-08 16:59 ` Brendan Jackman
2020-12-08 18:15 ` Yonghong Song
2020-12-15 11:12 ` Brendan Jackman
2020-12-16 7:18 ` Yonghong Song
2020-12-16 11:51 ` Brendan Jackman
2020-12-16 20:00 ` Yonghong Song
2020-12-07 16:07 ` [PATCH bpf-next v4 11/11] bpf: Document new atomic instructions Brendan Jackman
2020-12-08 3:25 ` Yonghong Song
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=X89G2kItO2o60+A6@google.com \
--to=jackmanb@google.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jannh@google.com \
--cc=john.fastabend@gmail.com \
--cc=kpsingh@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=revest@chromium.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.