From: John Fastabend <john.fastabend@gmail.com>
To: Brendan Jackman <jackmanb@google.com>, bpf@vger.kernel.org
Cc: 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>,
Brendan Jackman <jackmanb@google.com>
Subject: RE: [PATCH bpf-next v4 04/11] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
Date: Mon, 07 Dec 2020 13:56:53 -0800 [thread overview]
Message-ID: <5fcea525c4971_5a96208bd@john-XPS-13-9370.notmuch> (raw)
In-Reply-To: <20201207160734.2345502-5-jackmanb@google.com>
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.
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-07 21:57 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 [this message]
2020-12-08 9:26 ` Brendan Jackman
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=5fcea525c4971_5a96208bd@john-XPS-13-9370.notmuch \
--to=john.fastabend@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jackmanb@google.com \
--cc=jannh@google.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.