All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] Atomics for eBPF
@ 2020-11-23 17:31 Brendan Jackman
  2020-11-23 17:31 ` [PATCH 1/7] bpf: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
                   ` (7 more replies)
  0 siblings, 8 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:31 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest, Brendan Jackman

Status of the patches
=====================

This patchset is currently incomplete: it oughtn't be released as-is,
because that would necessitate incrementing architecture
version number for a partial feature-set. However I've implemented
the parts that I think are most interesting/controversial, so I'd
like to get the review started.

The missing elements that I'm aware of are:

* Atomic subtract
* Atomic & and |
* Documentation updates

The prog_test that's added depends on Clang/LLVM features added by
Yonghong in https://reviews.llvm.org/D72184

This only includes a JIT implementation for x86_64 - I don't plan to
implement JIT support myself for other architectures.

Operations
==========

This patchset adds atomic operations to the eBPF instruction set. The
use-case that motivated this work was a trivial and efficient way to
generate globally-unique cookies in BPF progs, but I think it's
obvious that these features are pretty widely applicable.  The
instructions that are added here can be summarised with this list of
kernel operations:

* atomic[64]_[fetch_]add
* atomic[64]_[fetch_]sub (TODO)
* atomic[64]_[fetch_]and (TODO)
* atomic[64]_[fetch_]or  (TODO)
* atomic[64]_xchg
* atomic[64]_cmpxchg

The following are left out of scope for this effort:

* 16 and 8 bit operations
* Explicit memory barriers

Encoding
========

I originally planned to add new values for bpf_insn.opcode. This was
rather unpleasant: the opcode space has holes in it but no entire
instruction classes[1]. Yonghong Song had a better idea: use the
immediate field of the existing STX XADD instruction to encode the
operation. This works nicely, without breaking existing programs,
because the immediate field is currently reserved-must-be-zero, and
extra-nicely because BPF_ADD happens to be zero.

Note that this of course makes immediate-source atomic operations
impossible. It's hard to imagine a measurable speedup from such
instructions, and if it existed it would certainly not benefit x86,
which has no support for them.

The BPF_OP opcode fields are re-used in the immediate, and an
additional flag BPF_FETCH is used to mark instructions that should
fetch a pre-modification value from memory.

So, BPF_XADD is now called BPF_ATOMIC (the old name is kept to avoid
breaking userspace builds), and where we previously had .imm = 0, we
now have .imm = BPF_ADD (which is 0).

Operands
========

Reg-source eBPF instructions only have two operands, while these
atomic operations have up to four. To avoid needing to encode
additional operands, then:

- One of the input registers is re-used as an output register
  (e.g. atomic_fetch_add both reads from and writes to the source
  register).

- Where necessary (i.e. for cmpxchg) , R0 is "hard-coded" as one of
  the operands.

This approach also allows the new eBPF instructions to map directly
to single x86 instructions.

[1] Visualisation of eBPF opcode space:
    https://gist.github.com/bjackman/00fdad2d5dfff601c1918bc29b16e778

Brendan Jackman (7):
  bpf: Factor out emission of ModR/M for *(reg + off)
  bpf: x86: Factor out emission of REX byte
  bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  bpf: Move BPF_STX reserved field check into BPF_STX verifier code
  bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
  bpf: Add instructions for atomic_cmpxchg and friends
  bpf: Add tests for new BPF atomic operations

 Documentation/networking/filter.rst           |  27 ++--
 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                  |  26 ++--
 arch/sparc/net/bpf_jit_comp_64.c              |  14 +-
 arch/x86/net/bpf_jit_comp.c                   | 131 ++++++++++------
 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 |  13 +-
 include/linux/filter.h                        |  47 +++++-
 include/uapi/linux/bpf.h                      |   9 +-
 kernel/bpf/core.c                             |  64 ++++++--
 kernel/bpf/disasm.c                           |  25 ++-
 kernel/bpf/verifier.c                         |  72 ++++++---
 lib/test_bpf.c                                |   2 +-
 samples/bpf/bpf_insn.h                        |   4 +-
 samples/bpf/sock_example.c                    |   3 +-
 samples/bpf/test_cgrp2_attach.c               |   6 +-
 tools/include/linux/filter.h                  |  47 +++++-
 tools/include/uapi/linux/bpf.h                |   9 +-
 tools/testing/selftests/bpf/Makefile          |   2 +-
 .../selftests/bpf/prog_tests/atomics_test.c   | 145 ++++++++++++++++++
 .../bpf/prog_tests/cgroup_attach_multi.c      |   6 +-
 .../selftests/bpf/progs/atomics_test.c        |  61 ++++++++
 .../selftests/bpf/verifier/atomic_cmpxchg.c   |  96 ++++++++++++
 .../selftests/bpf/verifier/atomic_fetch_add.c | 106 +++++++++++++
 .../selftests/bpf/verifier/atomic_xchg.c      | 113 ++++++++++++++
 tools/testing/selftests/bpf/verifier/ctx.c    |   6 +-
 .../testing/selftests/bpf/verifier/leak_ptr.c |   4 +-
 tools/testing/selftests/bpf/verifier/unpriv.c |   3 +-
 tools/testing/selftests/bpf/verifier/xadd.c   |   2 +-
 36 files changed, 1002 insertions(+), 160 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/atomics_test.c
 create mode 100644 tools/testing/selftests/bpf/progs/atomics_test.c
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_fetch_add.c
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_xchg.c

--
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 1/7] bpf: Factor out emission of ModR/M for *(reg + off)
  2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
@ 2020-11-23 17:31 ` Brendan Jackman
  2020-11-23 17:31 ` [PATCH 2/7] bpf: x86: Factor out emission of REX byte Brendan Jackman
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:31 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest, Brendan Jackman

The case for JITing atomics is about to get more complicated. Let's
factor out some common code to make the review and result more
readable.

NB the atomics code doesn't yet use the new helper - a subsequent
patch will add its use as a side-effect of other changes.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 arch/x86/net/bpf_jit_comp.c | 42 +++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 796506dcfc42..94b17bd30e00 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -681,6 +681,27 @@ static void emit_mov_reg(u8 **pprog, bool is64, u32 dst_reg, u32 src_reg)
 	*pprog = prog;
 }
 
+/* Emit the ModR/M byte for addressing *(r1 + off) and r2 */
+static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
+{
+	u8 *prog = *pprog;
+	int cnt = 0;
+
+	if (is_imm8(off)) {
+		/* 1-byte signed displacement.
+		 *
+		 * If off == 0 we could skip this and save one extra byte, but
+		 * special case of x86 R13 which always needs an offset is not
+		 * worth the hassle
+		 */
+		EMIT2(add_2reg(0x40, r1, r2), off);
+	} else {
+		/* 4-byte signed displacement */
+		EMIT1_off32(add_2reg(0x80, r1, r2), off);
+	}
+	*pprog = prog;
+}
+
 /* LDX: dst_reg = *(u8*)(src_reg + off) */
 static void emit_ldx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 {
@@ -708,15 +729,7 @@ static void emit_ldx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 		EMIT2(add_2mod(0x48, src_reg, dst_reg), 0x8B);
 		break;
 	}
-	/*
-	 * If insn->off == 0 we can save one extra byte, but
-	 * special case of x86 R13 which always needs an offset
-	 * is not worth the hassle
-	 */
-	if (is_imm8(off))
-		EMIT2(add_2reg(0x40, src_reg, dst_reg), off);
-	else
-		EMIT1_off32(add_2reg(0x80, src_reg, dst_reg), off);
+	emit_modrm_dstoff(&prog, src_reg, dst_reg, off);
 	*pprog = prog;
 }
 
@@ -751,10 +764,7 @@ static void emit_stx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 		EMIT2(add_2mod(0x48, dst_reg, src_reg), 0x89);
 		break;
 	}
-	if (is_imm8(off))
-		EMIT2(add_2reg(0x40, dst_reg, src_reg), off);
-	else
-		EMIT1_off32(add_2reg(0x80, dst_reg, src_reg), off);
+	emit_modrm_dstoff(&prog, dst_reg, src_reg, off);
 	*pprog = prog;
 }
 
@@ -1240,11 +1250,7 @@ st:			if (is_imm8(insn->off))
 			goto xadd;
 		case BPF_STX | BPF_XADD | BPF_DW:
 			EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
-xadd:			if (is_imm8(insn->off))
-				EMIT2(add_2reg(0x40, dst_reg, src_reg), insn->off);
-			else
-				EMIT1_off32(add_2reg(0x80, dst_reg, src_reg),
-					    insn->off);
+xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
 			break;
 
 			/* call */
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 2/7] bpf: x86: Factor out emission of REX byte
  2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
  2020-11-23 17:31 ` [PATCH 1/7] bpf: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
@ 2020-11-23 17:31 ` Brendan Jackman
  2020-11-23 17:31 ` [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:31 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest, Brendan Jackman

The JIT case for encoding atomic ops is about to get more
complicated. In order to make the review & resulting code easier,
let's factor out some shared helpers.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 arch/x86/net/bpf_jit_comp.c | 39 ++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 94b17bd30e00..a839c1a54276 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -702,6 +702,21 @@ static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
 	*pprog = prog;
 }
 
+/*
+ * Emit a REX byte if it will be necessary to address these registers
+ */
+static void maybe_emit_rex(u8 **pprog, u32 reg_rm, u32 reg_reg, bool wide)
+{
+	u8 *prog = *pprog;
+	int cnt = 0;
+
+	if (wide)
+		EMIT1(add_2mod(0x48, reg_rm, reg_reg));
+	else if (is_ereg(reg_rm) || is_ereg(reg_reg))
+		EMIT1(add_2mod(0x40, reg_rm, reg_reg));
+	*pprog = prog;
+}
+
 /* LDX: dst_reg = *(u8*)(src_reg + off) */
 static void emit_ldx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 {
@@ -854,10 +869,8 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			case BPF_OR: b2 = 0x09; break;
 			case BPF_XOR: b2 = 0x31; break;
 			}
-			if (BPF_CLASS(insn->code) == BPF_ALU64)
-				EMIT1(add_2mod(0x48, dst_reg, src_reg));
-			else if (is_ereg(dst_reg) || is_ereg(src_reg))
-				EMIT1(add_2mod(0x40, dst_reg, src_reg));
+			maybe_emit_rex(&prog, dst_reg, src_reg,
+				       BPF_CLASS(insn->code) == BPF_ALU64);
 			EMIT2(b2, add_2reg(0xC0, dst_reg, src_reg));
 			break;
 
@@ -1301,20 +1314,16 @@ xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
 		case BPF_JMP32 | BPF_JSGE | BPF_X:
 		case BPF_JMP32 | BPF_JSLE | BPF_X:
 			/* cmp dst_reg, src_reg */
-			if (BPF_CLASS(insn->code) == BPF_JMP)
-				EMIT1(add_2mod(0x48, dst_reg, src_reg));
-			else if (is_ereg(dst_reg) || is_ereg(src_reg))
-				EMIT1(add_2mod(0x40, dst_reg, src_reg));
+			maybe_emit_rex(&prog, dst_reg, src_reg,
+				       BPF_CLASS(insn->code) == BPF_JMP);
 			EMIT2(0x39, add_2reg(0xC0, dst_reg, src_reg));
 			goto emit_cond_jmp;
 
 		case BPF_JMP | BPF_JSET | BPF_X:
 		case BPF_JMP32 | BPF_JSET | BPF_X:
 			/* test dst_reg, src_reg */
-			if (BPF_CLASS(insn->code) == BPF_JMP)
-				EMIT1(add_2mod(0x48, dst_reg, src_reg));
-			else if (is_ereg(dst_reg) || is_ereg(src_reg))
-				EMIT1(add_2mod(0x40, dst_reg, src_reg));
+			maybe_emit_rex(&prog, dst_reg, src_reg,
+				       BPF_CLASS(insn->code) == BPF_JMP);
 			EMIT2(0x85, add_2reg(0xC0, dst_reg, src_reg));
 			goto emit_cond_jmp;
 
@@ -1350,10 +1359,8 @@ xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
 		case BPF_JMP32 | BPF_JSLE | BPF_K:
 			/* test dst_reg, dst_reg to save one extra byte */
 			if (imm32 == 0) {
-				if (BPF_CLASS(insn->code) == BPF_JMP)
-					EMIT1(add_2mod(0x48, dst_reg, dst_reg));
-				else if (is_ereg(dst_reg))
-					EMIT1(add_2mod(0x40, dst_reg, dst_reg));
+				maybe_emit_rex(&prog, dst_reg, dst_reg,
+					       BPF_CLASS(insn->code) == BPF_JMP);
 				EMIT2(0x85, add_2reg(0xC0, dst_reg, dst_reg));
 				goto emit_cond_jmp;
 			}
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
  2020-11-23 17:31 ` [PATCH 1/7] bpf: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
  2020-11-23 17:31 ` [PATCH 2/7] bpf: x86: Factor out emission of REX byte Brendan Jackman
@ 2020-11-23 17:31 ` Brendan Jackman
  2020-11-23 23:54   ` Yonghong Song
                     ` (2 more replies)
  2020-11-23 17:31 ` [PATCH 4/7] bpf: Move BPF_STX reserved field check into BPF_STX verifier code Brendan Jackman
                   ` (4 subsequent siblings)
  7 siblings, 3 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:31 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest, Brendan Jackman

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           | 27 +++++++++-------
 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                  | 26 +++++++++-------
 arch/sparc/net/bpf_jit_comp_64.c              | 14 +++++++--
 arch/x86/net/bpf_jit_comp.c                   | 30 +++++++++++-------
 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 | 13 +++++---
 include/linux/filter.h                        |  8 +++--
 include/uapi/linux/bpf.h                      |  3 +-
 kernel/bpf/core.c                             | 31 +++++++++++++------
 kernel/bpf/disasm.c                           |  6 ++--
 kernel/bpf/verifier.c                         | 24 ++++++++------
 lib/test_bpf.c                                |  2 +-
 samples/bpf/bpf_insn.h                        |  4 +--
 samples/bpf/sock_example.c                    |  3 +-
 samples/bpf/test_cgrp2_attach.c               |  6 ++--
 tools/include/linux/filter.h                  |  7 +++--
 tools/include/uapi/linux/bpf.h                |  3 +-
 .../bpf/prog_tests/cgroup_attach_multi.c      |  6 ++--
 tools/testing/selftests/bpf/verifier/ctx.c    |  6 ++--
 .../testing/selftests/bpf/verifier/leak_ptr.c |  4 +--
 tools/testing/selftests/bpf/verifier/unpriv.c |  3 +-
 tools/testing/selftests/bpf/verifier/xadd.c   |  2 +-
 30 files changed, 230 insertions(+), 117 deletions(-)

diff --git a/Documentation/networking/filter.rst b/Documentation/networking/filter.rst
index debb59e374de..a9847662bbab 100644
--- a/Documentation/networking/filter.rst
+++ b/Documentation/networking/filter.rst
@@ -1006,13 +1006,13 @@ Size modifier is one of ...
 
 Mode modifier is one of::
 
-  BPF_IMM  0x00  /* used for 32-bit mov in classic BPF and 64-bit in eBPF */
-  BPF_ABS  0x20
-  BPF_IND  0x40
-  BPF_MEM  0x60
-  BPF_LEN  0x80  /* classic BPF only, reserved in eBPF */
-  BPF_MSH  0xa0  /* classic BPF only, reserved in eBPF */
-  BPF_XADD 0xc0  /* eBPF only, exclusive add */
+  BPF_IMM     0x00  /* used for 32-bit mov in classic BPF and 64-bit in eBPF */
+  BPF_ABS     0x20
+  BPF_IND     0x40
+  BPF_MEM     0x60
+  BPF_LEN     0x80  /* classic BPF only, reserved in eBPF */
+  BPF_MSH     0xa0  /* classic BPF only, reserved in eBPF */
+  BPF_ATOMIC  0xc0  /* eBPF only, atomic operations */
 
 eBPF has two non-generic instructions: (BPF_ABS | <size> | BPF_LD) and
 (BPF_IND | <size> | BPF_LD) which are used to access packet data.
@@ -1044,11 +1044,16 @@ Unlike classic BPF instruction set, eBPF has generic load/store operations::
     BPF_MEM | <size> | BPF_STX:  *(size *) (dst_reg + off) = src_reg
     BPF_MEM | <size> | BPF_ST:   *(size *) (dst_reg + off) = imm32
     BPF_MEM | <size> | BPF_LDX:  dst_reg = *(size *) (src_reg + off)
-    BPF_XADD | BPF_W  | BPF_STX: lock xadd *(u32 *)(dst_reg + off16) += src_reg
-    BPF_XADD | BPF_DW | BPF_STX: lock xadd *(u64 *)(dst_reg + off16) += src_reg
 
-Where size is one of: BPF_B or BPF_H or BPF_W or BPF_DW. Note that 1 and
-2 byte atomic increments are not supported.
+Where size is one of: BPF_B or BPF_H or BPF_W or BPF_DW.
+
+It also includes atomic operations, which use the immediate field for extra
+encoding.
+
+   BPF_ADD, BPF_ATOMIC | BPF_W  | BPF_STX: lock xadd *(u32 *)(dst_reg + off16) += src_reg
+   BPF_ADD, BPF_ATOMIC | BPF_DW | BPF_STX: lock xadd *(u64 *)(dst_reg + off16) += src_reg
+
+Note that 1 and 2 byte atomic operations are not supported.
 
 eBPF has one 16-byte instruction: BPF_LD | BPF_DW | BPF_IMM which consists
 of two consecutive ``struct bpf_insn`` 8-byte blocks and interpreted as single
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 0207b6ea6e8a..897634d0a67c 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -1620,10 +1620,9 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 		}
 		emit_str_r(dst_lo, tmp2, off, ctx, BPF_SIZE(code));
 		break;
-	/* STX XADD: lock *(u32 *)(dst + off) += src */
-	case BPF_STX | BPF_XADD | BPF_W:
-	/* STX XADD: lock *(u64 *)(dst + off) += src */
-	case BPF_STX | BPF_XADD | BPF_DW:
+	/* Atomic ops */
+	case BPF_STX | BPF_ATOMIC | BPF_W:
+	case BPF_STX | BPF_ATOMIC | BPF_DW:
 		goto notyet;
 	/* STX: *(size *)(dst + off) = src */
 	case BPF_STX | BPF_MEM | BPF_W:
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index ef9f1d5e989d..f7b194878a99 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -875,10 +875,18 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
 		}
 		break;
 
-	/* STX XADD: lock *(u32 *)(dst + off) += src */
-	case BPF_STX | BPF_XADD | BPF_W:
-	/* STX XADD: lock *(u64 *)(dst + off) += src */
-	case BPF_STX | BPF_XADD | BPF_DW:
+	case BPF_STX | BPF_ATOMIC | BPF_W:
+	case BPF_STX | BPF_ATOMIC | BPF_DW:
+		if (insn->imm != BPF_ADD) {
+			pr_err_once("unknown atomic op code %02x\n", insn->imm);
+			return -EINVAL;
+		}
+
+		/* STX XADD: lock *(u32 *)(dst + off) += src
+		 * and
+		 * STX XADD: lock *(u64 *)(dst + off) += src
+		 */
+
 		if (!off) {
 			reg = dst;
 		} else {
diff --git a/arch/mips/net/ebpf_jit.c b/arch/mips/net/ebpf_jit.c
index 561154cbcc40..939dd06764bc 100644
--- a/arch/mips/net/ebpf_jit.c
+++ b/arch/mips/net/ebpf_jit.c
@@ -1423,8 +1423,8 @@ static int build_one_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
 	case BPF_STX | BPF_H | BPF_MEM:
 	case BPF_STX | BPF_W | BPF_MEM:
 	case BPF_STX | BPF_DW | BPF_MEM:
-	case BPF_STX | BPF_W | BPF_XADD:
-	case BPF_STX | BPF_DW | BPF_XADD:
+	case BPF_STX | BPF_W | BPF_ATOMIC:
+	case BPF_STX | BPF_DW | BPF_ATOMIC:
 		if (insn->dst_reg == BPF_REG_10) {
 			ctx->flags |= EBPF_SEEN_FP;
 			dst = MIPS_R_SP;
@@ -1438,7 +1438,12 @@ static int build_one_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
 		src = ebpf_to_mips_reg(ctx, insn, src_reg_no_fp);
 		if (src < 0)
 			return src;
-		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;
+			}
+
 			/*
 			 * If mem_off does not fit within the 9 bit ll/sc
 			 * instruction immediate field, use a temp reg.
diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index 022103c6a201..aaf1a887f653 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -683,10 +683,18 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
 			break;
 
 		/*
-		 * BPF_STX XADD (atomic_add)
+		 * BPF_STX ATOMIC (atomic ops)
 		 */
-		/* *(u32 *)(dst + off) += src */
-		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;
+			}
+
+			/* *(u32 *)(dst + off) += src */
+
 			/* Get EA into TMP_REG_1 */
 			EMIT(PPC_RAW_ADDI(b2p[TMP_REG_1], dst_reg, off));
 			tmp_idx = ctx->idx * 4;
@@ -699,8 +707,15 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
 			/* we're done if this succeeded */
 			PPC_BCC_SHORT(COND_NE, tmp_idx);
 			break;
-		/* *(u64 *)(dst + off) += src */
-		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;
+			}
+			/* *(u64 *)(dst + off) += src */
+
 			EMIT(PPC_RAW_ADDI(b2p[TMP_REG_1], dst_reg, off));
 			tmp_idx = ctx->idx * 4;
 			EMIT(PPC_RAW_LDARX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1], 0));
diff --git a/arch/riscv/net/bpf_jit_comp32.c b/arch/riscv/net/bpf_jit_comp32.c
index 579575f9cdae..a604e0fe2015 100644
--- a/arch/riscv/net/bpf_jit_comp32.c
+++ b/arch/riscv/net/bpf_jit_comp32.c
@@ -881,7 +881,7 @@ static int emit_store_r64(const s8 *dst, const s8 *src, s16 off,
 	const s8 *rd = bpf_get_reg64(dst, tmp1, ctx);
 	const s8 *rs = bpf_get_reg64(src, tmp2, ctx);
 
-	if (mode == BPF_XADD && size != BPF_W)
+	if (mode == BPF_ATOMIC && (size != BPF_W || imm != BPF_ADD))
 		return -1;
 
 	emit_imm(RV_REG_T0, off, ctx);
@@ -899,7 +899,7 @@ static int emit_store_r64(const s8 *dst, const s8 *src, s16 off,
 		case BPF_MEM:
 			emit(rv_sw(RV_REG_T0, 0, lo(rs)), ctx);
 			break;
-		case BPF_XADD:
+		case BPF_ATOMIC: /* .imm checked above. This is XADD */
 			emit(rv_amoadd_w(RV_REG_ZERO, lo(rs), RV_REG_T0, 0, 0),
 			     ctx);
 			break;
@@ -1260,7 +1260,6 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 	case BPF_STX | BPF_MEM | BPF_H:
 	case BPF_STX | BPF_MEM | BPF_W:
 	case BPF_STX | BPF_MEM | BPF_DW:
-	case BPF_STX | BPF_XADD | BPF_W:
 		if (BPF_CLASS(code) == BPF_ST) {
 			emit_imm32(tmp2, imm, ctx);
 			src = tmp2;
@@ -1271,8 +1270,21 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 			return -1;
 		break;
 
+	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;
+		}
+
+		if (emit_store_r64(dst, src, off, ctx, BPF_SIZE(code),
+				   BPF_MODE(code)))
+			return -1;
+		break;
+
 	/* No hardware support for 8-byte atomics in RV32. */
-	case BPF_STX | BPF_XADD | BPF_DW:
+	case BPF_STX | BPF_ATOMIC | BPF_DW:
 		/* Fallthrough. */
 
 notsupported:
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 8a56b5293117..7696b2baf915 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -1027,10 +1027,18 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 		emit_add(RV_REG_T1, RV_REG_T1, rd, ctx);
 		emit_sd(RV_REG_T1, 0, rs, ctx);
 		break;
-	/* STX XADD: lock *(u32 *)(dst + off) += src */
-	case BPF_STX | BPF_XADD | BPF_W:
-	/* STX XADD: lock *(u64 *)(dst + off) += src */
-	case BPF_STX | BPF_XADD | BPF_DW:
+	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;
+		}
+
+		/* STX XADD: lock *(u32 *)(dst + off) += src
+		 * STX XADD: lock *(u64 *)(dst + off) += src
+		 */
+
 		if (off) {
 			if (is_12b_int(off)) {
 				emit_addi(RV_REG_T1, rd, off, ctx);
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 0a4182792876..d02eae46be39 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -1205,18 +1205,22 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		jit->seen |= SEEN_MEM;
 		break;
 	/*
-	 * BPF_STX XADD (atomic_add)
+	 * BPF_STX ATM (atomic ops)
 	 */
-	case BPF_STX | BPF_XADD | BPF_W: /* *(u32 *)(dst + off) += src */
-		/* laal %w0,%src,off(%dst) */
-		EMIT6_DISP_LH(0xeb000000, 0x00fa, REG_W0, src_reg,
-			      dst_reg, off);
-		jit->seen |= SEEN_MEM;
-		break;
-	case BPF_STX | BPF_XADD | BPF_DW: /* *(u64 *)(dst + off) += src */
-		/* laalg %w0,%src,off(%dst) */
-		EMIT6_DISP_LH(0xeb000000, 0x00ea, REG_W0, src_reg,
-			      dst_reg, off);
+	case BPF_STX | BPF_ATOMIC | BPF_W:
+		if (insn->imm != BPF_ADD) {
+			pr_err("Unknown atomic operation %02x\n", insn->imm);
+			return -1;
+		}
+
+		/* *(u32/u64 *)(dst + off) += src
+		 *
+		 * BFW_W:  laal  %w0,%src,off(%dst)
+		 * BPF_DW: laalg %w0,%src,off(%dst)
+		 */
+		EMIT6_DISP_LH(0xeb000000,
+			      BPF_SIZE(insn->code) == BPF_W ? 0x00fa : 0x00ea,
+			      REG_W0, src_reg, dst_reg, off);
 		jit->seen |= SEEN_MEM;
 		break;
 	/*
diff --git a/arch/sparc/net/bpf_jit_comp_64.c b/arch/sparc/net/bpf_jit_comp_64.c
index 3364e2a00989..4fa4ad61dd35 100644
--- a/arch/sparc/net/bpf_jit_comp_64.c
+++ b/arch/sparc/net/bpf_jit_comp_64.c
@@ -1367,11 +1367,16 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 	}
 
 	/* STX XADD: lock *(u32 *)(dst + off) += src */
-	case BPF_STX | BPF_XADD | BPF_W: {
+	case BPF_STX | BPF_ATOMIC | BPF_W: {
 		const u8 tmp = bpf2sparc[TMP_REG_1];
 		const u8 tmp2 = bpf2sparc[TMP_REG_2];
 		const u8 tmp3 = bpf2sparc[TMP_REG_3];
 
+		if (insn->imm != BPF_ADD) {
+			pr_err_once("unknown atomic op %02x\n", insn->imm);
+			return -EINVAL;
+		}
+
 		if (insn->dst_reg == BPF_REG_FP)
 			ctx->saw_frame_pointer = true;
 
@@ -1390,11 +1395,16 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 		break;
 	}
 	/* STX XADD: lock *(u64 *)(dst + off) += src */
-	case BPF_STX | BPF_XADD | BPF_DW: {
+	case BPF_STX | BPF_ATOMIC | BPF_DW: {
 		const u8 tmp = bpf2sparc[TMP_REG_1];
 		const u8 tmp2 = bpf2sparc[TMP_REG_2];
 		const u8 tmp3 = bpf2sparc[TMP_REG_3];
 
+		if (insn->imm != BPF_ADD) {
+			pr_err_once("unknown atomic op %02x\n", insn->imm);
+			return -EINVAL;
+		}
+
 		if (insn->dst_reg == BPF_REG_FP)
 			ctx->saw_frame_pointer = true;
 
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a839c1a54276..0ff2416d99b6 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1253,17 +1253,25 @@ st:			if (is_imm8(insn->off))
 			}
 			break;
 
-			/* STX XADD: lock *(u32*)(dst_reg + off) += src_reg */
-		case BPF_STX | BPF_XADD | BPF_W:
-			/* Emit 'lock add dword ptr [rax + off], eax' */
-			if (is_ereg(dst_reg) || is_ereg(src_reg))
-				EMIT3(0xF0, add_2mod(0x40, dst_reg, src_reg), 0x01);
-			else
-				EMIT2(0xF0, 0x01);
-			goto xadd;
-		case BPF_STX | BPF_XADD | BPF_DW:
-			EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
-xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
+		case BPF_STX | BPF_ATOMIC | BPF_W:
+		case BPF_STX | BPF_ATOMIC | BPF_DW:
+			if (insn->imm != BPF_ADD) {
+				pr_err("bpf_jit: unknown opcode %02x\n", insn->imm);
+				return -EFAULT;
+			}
+
+			/* XADD: lock *(u32/u64*)(dst_reg + off) += src_reg */
+
+			if (BPF_SIZE(insn->code) == BPF_W) {
+				/* Emit 'lock add dword ptr [rax + off], eax' */
+				if (is_ereg(dst_reg) || is_ereg(src_reg))
+					EMIT3(0xF0, add_2mod(0x40, dst_reg, src_reg), 0x01);
+				else
+					EMIT2(0xF0, 0x01);
+			} else {
+				EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
+			}
+			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
 			break;
 
 			/* call */
diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
index 96fde03aa987..d17b67c69f89 100644
--- a/arch/x86/net/bpf_jit_comp32.c
+++ b/arch/x86/net/bpf_jit_comp32.c
@@ -2243,10 +2243,8 @@ emit_cond_jmp:		jmp_cond = get_cond_jmp_opcode(BPF_OP(code), false);
 				return -EFAULT;
 			}
 			break;
-		/* STX XADD: lock *(u32 *)(dst + off) += src */
-		case BPF_STX | BPF_XADD | BPF_W:
-		/* STX XADD: lock *(u64 *)(dst + off) += src */
-		case BPF_STX | BPF_XADD | BPF_DW:
+		case BPF_STX | BPF_ATOMIC | BPF_W:
+		case BPF_STX | BPF_ATOMIC | BPF_DW:
 			goto notyet;
 		case BPF_JMP | BPF_EXIT:
 			if (seen_exit) {
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index 0a721f6e8676..0767d7b579e9 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -3109,13 +3109,19 @@ mem_xadd(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta, bool is64)
 	return 0;
 }
 
-static int mem_xadd4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
+static int mem_atm4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
 {
+	if (meta->insn.off != BPF_ADD)
+		return -EOPNOTSUPP;
+
 	return mem_xadd(nfp_prog, meta, false);
 }
 
-static int mem_xadd8(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
+static int mem_atm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
 {
+	if (meta->insn.off != BPF_ADD)
+		return -EOPNOTSUPP;
+
 	return mem_xadd(nfp_prog, meta, true);
 }
 
@@ -3475,8 +3481,8 @@ static const instr_cb_t instr_cb[256] = {
 	[BPF_STX | BPF_MEM | BPF_H] =	mem_stx2,
 	[BPF_STX | BPF_MEM | BPF_W] =	mem_stx4,
 	[BPF_STX | BPF_MEM | BPF_DW] =	mem_stx8,
-	[BPF_STX | BPF_XADD | BPF_W] =	mem_xadd4,
-	[BPF_STX | BPF_XADD | BPF_DW] =	mem_xadd8,
+	[BPF_STX | BPF_ATOMIC | BPF_W] =	mem_atm4,
+	[BPF_STX | BPF_ATOMIC | BPF_DW] =	mem_atm8,
 	[BPF_ST | BPF_MEM | BPF_B] =	mem_st1,
 	[BPF_ST | BPF_MEM | BPF_H] =	mem_st2,
 	[BPF_ST | BPF_MEM | BPF_W] =	mem_st4,
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index fac9c6f9e197..e9e8ff0e7ae9 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -428,9 +428,9 @@ static inline bool is_mbpf_classic_store_pkt(const struct nfp_insn_meta *meta)
 	return is_mbpf_classic_store(meta) && meta->ptr.type == PTR_TO_PACKET;
 }
 
-static inline bool is_mbpf_xadd(const struct nfp_insn_meta *meta)
+static inline bool is_mbpf_atm(const struct nfp_insn_meta *meta)
 {
-	return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_XADD);
+	return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_ATOMIC);
 }
 
 static inline bool is_mbpf_mul(const struct nfp_insn_meta *meta)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index e92ee510fd52..431b2a957139 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -523,12 +523,17 @@ nfp_bpf_check_store(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
 }
 
 static int
-nfp_bpf_check_xadd(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
-		   struct bpf_verifier_env *env)
+nfp_bpf_check_atm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
+		  struct bpf_verifier_env *env)
 {
 	const struct bpf_reg_state *sreg = cur_regs(env) + meta->insn.src_reg;
 	const struct bpf_reg_state *dreg = cur_regs(env) + meta->insn.dst_reg;
 
+	if (meta->insn.imm != BPF_ADD) {
+		pr_vlog(env, "atomic op not implemented: %d\n", meta->insn.imm);
+		return -EOPNOTSUPP;
+	}
+
 	if (dreg->type != PTR_TO_MAP_VALUE) {
 		pr_vlog(env, "atomic add not to a map value pointer: %d\n",
 			dreg->type);
@@ -655,8 +660,8 @@ int nfp_verify_insn(struct bpf_verifier_env *env, int insn_idx,
 	if (is_mbpf_store(meta))
 		return nfp_bpf_check_store(nfp_prog, meta, env);
 
-	if (is_mbpf_xadd(meta))
-		return nfp_bpf_check_xadd(nfp_prog, meta, env);
+	if (is_mbpf_atm(meta))
+		return nfp_bpf_check_atm(nfp_prog, meta, env);
 
 	if (is_mbpf_alu(meta))
 		return nfp_bpf_check_alu(nfp_prog, meta, env);
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 1b62397bd124..ce19988fb312 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -261,13 +261,15 @@ static inline bool insn_is_zext(const struct bpf_insn *insn)
 
 /* Atomic memory add, *(uint *)(dst_reg + off16) += src_reg */
 
-#define BPF_STX_XADD(SIZE, DST, SRC, OFF)			\
+#define BPF_ATOMIC_ADD(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 })
+#define BPF_STX_XADD BPF_ATOMIC_ADD /* alias */
+
 
 /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 3ca6146f001a..dcd08783647d 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -19,7 +19,8 @@
 
 /* ld/ldx fields */
 #define BPF_DW		0x18	/* double word (64-bit) */
-#define BPF_XADD	0xc0	/* exclusive add */
+#define BPF_ATOMIC	0xc0	/* atomic memory ops - op type in immediate */
+#define BPF_XADD	0xc0	/* legacy name, don't add new uses */
 
 /* alu/jmp fields */
 #define BPF_MOV		0xb0	/* mov reg to reg */
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ff55cbcfbab4..48b192a8edce 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1317,8 +1317,8 @@ EXPORT_SYMBOL_GPL(__bpf_call_base);
 	INSN_3(STX, MEM,  H),			\
 	INSN_3(STX, MEM,  W),			\
 	INSN_3(STX, MEM,  DW),			\
-	INSN_3(STX, XADD, W),			\
-	INSN_3(STX, XADD, DW),			\
+	INSN_3(STX, ATOMIC, W),			\
+	INSN_3(STX, ATOMIC, DW),		\
 	/*   Immediate based. */		\
 	INSN_3(ST, MEM, B),			\
 	INSN_3(ST, MEM, H),			\
@@ -1626,13 +1626,25 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 	LDX_PROBE(DW, 8)
 #undef LDX_PROBE
 
-	STX_XADD_W: /* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
-		atomic_add((u32) SRC, (atomic_t *)(unsigned long)
-			   (DST + insn->off));
+	STX_ATOMIC_W:
+		switch (insn->imm) {
+		case BPF_ADD:
+			/* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
+			atomic_add((u32) SRC, (atomic_t *)(unsigned long)
+				   (DST + insn->off));
+		default:
+			goto default_label;
+		}
 		CONT;
-	STX_XADD_DW: /* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
-		atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
-			     (DST + insn->off));
+	STX_ATOMIC_DW:
+		switch (insn->imm) {
+		case BPF_ADD:
+			/* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
+			atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
+				     (DST + insn->off));
+		default:
+			goto default_label;
+		}
 		CONT;
 
 	default_label:
@@ -1642,7 +1654,8 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 		 *
 		 * Note, verifier whitelists all opcodes in bpf_opcode_in_insntable().
 		 */
-		pr_warn("BPF interpreter: unknown opcode %02x\n", insn->code);
+		pr_warn("BPF interpreter: unknown opcode %02x (imm: 0x%x)\n",
+			insn->code, insn->imm);
 		BUG_ON(1);
 		return 0;
 }
diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c
index b44d8c447afd..37c8d6e9b4cc 100644
--- a/kernel/bpf/disasm.c
+++ b/kernel/bpf/disasm.c
@@ -153,14 +153,16 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
 				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
 				insn->dst_reg,
 				insn->off, insn->src_reg);
-		else if (BPF_MODE(insn->code) == BPF_XADD)
+		else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
+			 insn->imm == BPF_ADD) {
 			verbose(cbs->private_data, "(%02x) lock *(%s *)(r%d %+d) += r%d\n",
 				insn->code,
 				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
 				insn->dst_reg, insn->off,
 				insn->src_reg);
-		else
+		} else {
 			verbose(cbs->private_data, "BUG_%02x\n", insn->code);
+		}
 	} else if (class == BPF_ST) {
 		if (BPF_MODE(insn->code) != BPF_MEM) {
 			verbose(cbs->private_data, "BUG_st_%02x\n", insn->code);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fb2943ea715d..06885e2501f8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3598,13 +3598,17 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 	return err;
 }
 
-static int check_xadd(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
+static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
 {
 	int err;
 
-	if ((BPF_SIZE(insn->code) != BPF_W && BPF_SIZE(insn->code) != BPF_DW) ||
-	    insn->imm != 0) {
-		verbose(env, "BPF_XADD uses reserved fields\n");
+	if (insn->imm != BPF_ADD) {
+		verbose(env, "BPF_ATOMIC uses invalid atomic opcode %02x\n", insn->imm);
+		return -EINVAL;
+	}
+
+	if (BPF_SIZE(insn->code) != BPF_W && BPF_SIZE(insn->code) != BPF_DW) {
+		verbose(env, "invalid atomic operand size\n");
 		return -EINVAL;
 	}
 
@@ -3627,19 +3631,19 @@ static int check_xadd(struct bpf_verifier_env *env, int insn_idx, struct bpf_ins
 	    is_pkt_reg(env, insn->dst_reg) ||
 	    is_flow_key_reg(env, insn->dst_reg) ||
 	    is_sk_reg(env, insn->dst_reg)) {
-		verbose(env, "BPF_XADD stores into R%d %s is not allowed\n",
+		verbose(env, "atomic stores into R%d %s is not allowed\n",
 			insn->dst_reg,
 			reg_type_str[reg_state(env, insn->dst_reg)->type]);
 		return -EACCES;
 	}
 
-	/* check whether atomic_add can read the memory */
+	/* check whether we can read the memory */
 	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
 			       BPF_SIZE(insn->code), BPF_READ, -1, true);
 	if (err)
 		return err;
 
-	/* check whether atomic_add can write into the same memory */
+	/* check whether we can write into the same memory */
 	return check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
 				BPF_SIZE(insn->code), BPF_WRITE, -1, true);
 }
@@ -9486,8 +9490,8 @@ static int do_check(struct bpf_verifier_env *env)
 		} else if (class == BPF_STX) {
 			enum bpf_reg_type *prev_dst_type, dst_reg_type;
 
-			if (BPF_MODE(insn->code) == BPF_XADD) {
-				err = check_xadd(env, env->insn_idx, insn);
+			if (BPF_MODE(insn->code) == BPF_ATOMIC) {
+				err = check_atomic(env, env->insn_idx, insn);
 				if (err)
 					return err;
 				env->insn_idx++;
@@ -9897,7 +9901,7 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 
 		if (BPF_CLASS(insn->code) == BPF_STX &&
 		    ((BPF_MODE(insn->code) != BPF_MEM &&
-		      BPF_MODE(insn->code) != BPF_XADD) || insn->imm != 0)) {
+		      BPF_MODE(insn->code) != BPF_ATOMIC) || insn->imm != 0)) {
 			verbose(env, "BPF_STX uses reserved fields\n");
 			return -EINVAL;
 		}
diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index ca7d635bccd9..fbb13ef9207c 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -4295,7 +4295,7 @@ static struct bpf_test tests[] = {
 		{ { 0, 0xffffffff } },
 		.stack_depth = 40,
 	},
-	/* BPF_STX | BPF_XADD | BPF_W/DW */
+	/* BPF_STX | BPF_ATOMIC | BPF_W/DW */
 	{
 		"STX_XADD_W: Test: 0x12 + 0x10 = 0x22",
 		.u.insns_int = {
diff --git a/samples/bpf/bpf_insn.h b/samples/bpf/bpf_insn.h
index 544237980582..db67a2847395 100644
--- a/samples/bpf/bpf_insn.h
+++ b/samples/bpf/bpf_insn.h
@@ -138,11 +138,11 @@ struct bpf_insn;
 
 #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 */
 
diff --git a/samples/bpf/sock_example.c b/samples/bpf/sock_example.c
index 00aae1d33fca..41ec3ca3bc40 100644
--- a/samples/bpf/sock_example.c
+++ b/samples/bpf/sock_example.c
@@ -54,7 +54,8 @@ static int test_sock(void)
 		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
 		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
 		BPF_MOV64_IMM(BPF_REG_1, 1), /* r1 = 1 */
-		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
+		BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
+			     BPF_REG_0, BPF_REG_1, 0, BPF_ADD), /* xadd r0 += r1 */
 		BPF_MOV64_IMM(BPF_REG_0, 0), /* r0 = 0 */
 		BPF_EXIT_INSN(),
 	};
diff --git a/samples/bpf/test_cgrp2_attach.c b/samples/bpf/test_cgrp2_attach.c
index 20fbd1241db3..aab0ef301536 100644
--- a/samples/bpf/test_cgrp2_attach.c
+++ b/samples/bpf/test_cgrp2_attach.c
@@ -53,7 +53,8 @@ static int prog_load(int map_fd, int verdict)
 		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
 		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
 		BPF_MOV64_IMM(BPF_REG_1, 1), /* r1 = 1 */
-		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
+		BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
+			     BPF_REG_0, BPF_REG_1, 0, BPF_ADD), /* xadd r0 += r1 */
 
 		/* Count bytes */
 		BPF_MOV64_IMM(BPF_REG_0, MAP_KEY_BYTES), /* r0 = 1 */
@@ -64,7 +65,8 @@ static int prog_load(int map_fd, int verdict)
 		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
 		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
 		BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_6, offsetof(struct __sk_buff, len)), /* r1 = skb->len */
-		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
+		BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
+			     BPF_REG_0, BPF_REG_1, 0, BPF_ADD), /* xadd r0 += r1 */
 
 		BPF_MOV64_IMM(BPF_REG_0, verdict), /* r0 = verdict */
 		BPF_EXIT_INSN(),
diff --git a/tools/include/linux/filter.h b/tools/include/linux/filter.h
index ca28b6ab8db7..95ff51d97f25 100644
--- a/tools/include/linux/filter.h
+++ b/tools/include/linux/filter.h
@@ -171,13 +171,14 @@
 
 /* Atomic memory add, *(uint *)(dst_reg + off16) += src_reg */
 
-#define BPF_STX_XADD(SIZE, DST, SRC, OFF)			\
+#define BPF_ATOMIC_ADD(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 })
+#define BPF_STX_XADD BPF_ATOMIC_ADD /* alias */
 
 /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
 
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 3ca6146f001a..dcd08783647d 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -19,7 +19,8 @@
 
 /* ld/ldx fields */
 #define BPF_DW		0x18	/* double word (64-bit) */
-#define BPF_XADD	0xc0	/* exclusive add */
+#define BPF_ATOMIC	0xc0	/* atomic memory ops - op type in immediate */
+#define BPF_XADD	0xc0	/* legacy name, don't add new uses */
 
 /* alu/jmp fields */
 #define BPF_MOV		0xb0	/* mov reg to reg */
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
index b549fcfacc0b..79401a59a988 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
@@ -45,13 +45,15 @@ static int prog_load_cnt(int verdict, int val)
 		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
 		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
 		BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = 1 */
-		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
+		BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
+			     BPF_REG_0, BPF_REG_1, 0, BPF_ADD), /* xadd r0 += r1 */
 
 		BPF_LD_MAP_FD(BPF_REG_1, cgroup_storage_fd),
 		BPF_MOV64_IMM(BPF_REG_2, 0),
 		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_local_storage),
 		BPF_MOV64_IMM(BPF_REG_1, val),
-		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_W, BPF_REG_0, BPF_REG_1, 0, 0),
+		BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_W,
+			     BPF_REG_0, BPF_REG_1, 0, BPF_ADD),
 
 		BPF_LD_MAP_FD(BPF_REG_1, percpu_cgroup_storage_fd),
 		BPF_MOV64_IMM(BPF_REG_2, 0),
diff --git a/tools/testing/selftests/bpf/verifier/ctx.c b/tools/testing/selftests/bpf/verifier/ctx.c
index 93d6b1641481..0546d91d38cb 100644
--- a/tools/testing/selftests/bpf/verifier/ctx.c
+++ b/tools/testing/selftests/bpf/verifier/ctx.c
@@ -13,11 +13,11 @@
 	"context stores via XADD",
 	.insns = {
 	BPF_MOV64_IMM(BPF_REG_0, 0),
-	BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_W, BPF_REG_1,
-		     BPF_REG_0, offsetof(struct __sk_buff, mark), 0),
+	BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_W, BPF_REG_1,
+		     BPF_REG_0, offsetof(struct __sk_buff, mark), BPF_ADD),
 	BPF_EXIT_INSN(),
 	},
-	.errstr = "BPF_XADD stores into R1 ctx is not allowed",
+	.errstr = "BPF_ATOMIC stores into R1 ctx is not allowed",
 	.result = REJECT,
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 },
diff --git a/tools/testing/selftests/bpf/verifier/leak_ptr.c b/tools/testing/selftests/bpf/verifier/leak_ptr.c
index d6eec17f2cd2..f9a594b48fb3 100644
--- a/tools/testing/selftests/bpf/verifier/leak_ptr.c
+++ b/tools/testing/selftests/bpf/verifier/leak_ptr.c
@@ -13,7 +13,7 @@
 	.errstr_unpriv = "R2 leaks addr into mem",
 	.result_unpriv = REJECT,
 	.result = REJECT,
-	.errstr = "BPF_XADD stores into R1 ctx is not allowed",
+	.errstr = "BPF_ATOMIC stores into R1 ctx is not allowed",
 },
 {
 	"leak pointer into ctx 2",
@@ -28,7 +28,7 @@
 	.errstr_unpriv = "R10 leaks addr into mem",
 	.result_unpriv = REJECT,
 	.result = REJECT,
-	.errstr = "BPF_XADD stores into R1 ctx is not allowed",
+	.errstr = "BPF_ATOMIC stores into R1 ctx is not allowed",
 },
 {
 	"leak pointer into ctx 3",
diff --git a/tools/testing/selftests/bpf/verifier/unpriv.c b/tools/testing/selftests/bpf/verifier/unpriv.c
index 91bb77c24a2e..85b5e8b70e5d 100644
--- a/tools/testing/selftests/bpf/verifier/unpriv.c
+++ b/tools/testing/selftests/bpf/verifier/unpriv.c
@@ -206,7 +206,8 @@
 	BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, -8),
 	BPF_STX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, 0),
 	BPF_MOV64_IMM(BPF_REG_0, 1),
-	BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_10, BPF_REG_0, -8, 0),
+	BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
+		     BPF_REG_10, BPF_REG_0, -8, BPF_ADD),
 	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_6, 0),
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_hash_recalc),
 	BPF_EXIT_INSN(),
diff --git a/tools/testing/selftests/bpf/verifier/xadd.c b/tools/testing/selftests/bpf/verifier/xadd.c
index c5de2e62cc8b..70a320505bf2 100644
--- a/tools/testing/selftests/bpf/verifier/xadd.c
+++ b/tools/testing/selftests/bpf/verifier/xadd.c
@@ -51,7 +51,7 @@
 	BPF_EXIT_INSN(),
 	},
 	.result = REJECT,
-	.errstr = "BPF_XADD stores into R2 pkt is not allowed",
+	.errstr = "BPF_ATOMIC stores into R2 pkt is not allowed",
 	.prog_type = BPF_PROG_TYPE_XDP,
 	.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
 },
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 4/7] bpf: Move BPF_STX reserved field check into BPF_STX verifier code
  2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
                   ` (2 preceding siblings ...)
  2020-11-23 17:31 ` [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
@ 2020-11-23 17:31 ` Brendan Jackman
  2020-11-23 17:32 ` [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:31 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest, Brendan Jackman

I can't find a reason why this code is in resolve_pseudo_ldimm64;
since I'll be modifying it in a subsequent commit, tidy it up.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 kernel/bpf/verifier.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 06885e2501f8..609cc5e9571f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9490,6 +9490,12 @@ static int do_check(struct bpf_verifier_env *env)
 		} else if (class == BPF_STX) {
 			enum bpf_reg_type *prev_dst_type, dst_reg_type;
 
+			if (((BPF_MODE(insn->code) != BPF_MEM &&
+			      BPF_MODE(insn->code) != BPF_ATOMIC) || insn->imm != 0)) {
+				verbose(env, "BPF_STX uses reserved fields\n");
+				return -EINVAL;
+			}
+
 			if (BPF_MODE(insn->code) == BPF_ATOMIC) {
 				err = check_atomic(env, env->insn_idx, insn);
 				if (err)
@@ -9899,13 +9905,6 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 			return -EINVAL;
 		}
 
-		if (BPF_CLASS(insn->code) == BPF_STX &&
-		    ((BPF_MODE(insn->code) != BPF_MEM &&
-		      BPF_MODE(insn->code) != BPF_ATOMIC) || insn->imm != 0)) {
-			verbose(env, "BPF_STX uses reserved fields\n");
-			return -EINVAL;
-		}
-
 		if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW)) {
 			struct bpf_insn_aux_data *aux;
 			struct bpf_map *map;
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
  2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
                   ` (3 preceding siblings ...)
  2020-11-23 17:31 ` [PATCH 4/7] bpf: Move BPF_STX reserved field check into BPF_STX verifier code Brendan Jackman
@ 2020-11-23 17:32 ` Brendan Jackman
  2020-11-23 21:12     ` kernel test robot
                     ` (2 more replies)
  2020-11-23 17:32 ` [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends Brendan Jackman
                   ` (2 subsequent siblings)
  7 siblings, 3 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:32 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest, Brendan Jackman

This value can be set in bpf_insn.imm, for BPF_ATOMIC instructions,
in order to have the previous value of the atomically-modified memory
location loaded into the src register after an atomic op is carried
out.

Suggested-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 arch/x86/net/bpf_jit_comp.c    | 21 ++++++++++---------
 include/linux/filter.h         |  9 +++++++++
 include/uapi/linux/bpf.h       |  3 +++
 kernel/bpf/core.c              | 17 ++++++++++++++--
 kernel/bpf/disasm.c            |  6 ++++++
 kernel/bpf/verifier.c          | 37 +++++++++++++++++++++++++---------
 tools/include/linux/filter.h   | 10 ++++++++-
 tools/include/uapi/linux/bpf.h |  3 +++
 8 files changed, 84 insertions(+), 22 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 0ff2416d99b6..b475bf525424 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1255,22 +1255,25 @@ st:			if (is_imm8(insn->off))
 
 		case BPF_STX | BPF_ATOMIC | BPF_W:
 		case BPF_STX | BPF_ATOMIC | BPF_DW:
-			if (insn->imm != BPF_ADD) {
+			if (BPF_OP(insn->imm) != BPF_ADD) {
 				pr_err("bpf_jit: unknown opcode %02x\n", insn->imm);
 				return -EFAULT;
 			}
 
-			/* XADD: lock *(u32/u64*)(dst_reg + off) += src_reg */
+			EMIT1(0xF0); /* lock prefix */
 
-			if (BPF_SIZE(insn->code) == BPF_W) {
-				/* Emit 'lock add dword ptr [rax + off], eax' */
-				if (is_ereg(dst_reg) || is_ereg(src_reg))
-					EMIT3(0xF0, add_2mod(0x40, dst_reg, src_reg), 0x01);
-				else
-					EMIT2(0xF0, 0x01);
+			maybe_emit_rex(&prog, dst_reg, src_reg,
+				       BPF_SIZE(insn->code) == BPF_DW);
+
+			/* emit opcode */
+			if (insn->imm & BPF_FETCH) {
+				/* src_reg = sync_fetch_and_add(*(dst_reg + off), src_reg); */
+				EMIT2(0x0F, 0xC1);
 			} else {
-				EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
+				/* lock *(u32/u64*)(dst_reg + off) += src_reg */
+				EMIT1(0x01);
 			}
+
 			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
 			break;
 
diff --git a/include/linux/filter.h b/include/linux/filter.h
index ce19988fb312..bf0ff3649f46 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -270,6 +270,15 @@ static inline bool insn_is_zext(const struct bpf_insn *insn)
 		.imm   = BPF_ADD })
 #define BPF_STX_XADD BPF_ATOMIC_ADD /* alias */
 
+/* Atomic memory add with fetch, src_reg = sync_fetch_and_add(*(dst_reg + off), src_reg); */
+
+#define BPF_ATOMIC_FETCH_ADD(SIZE, DST, SRC, OFF)		\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = BPF_ADD | BPF_FETCH })
 
 /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index dcd08783647d..ec7f415f331b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -44,6 +44,9 @@
 #define BPF_CALL	0x80	/* function call */
 #define BPF_EXIT	0x90	/* function return */
 
+/* atomic op type fields (stored in immediate) */
+#define BPF_FETCH	0x01	/* fetch previous value into src reg */
+
 /* Register numbers */
 enum {
 	BPF_REG_0 = 0,
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 48b192a8edce..49a2a533db60 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1627,21 +1627,34 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #undef LDX_PROBE
 
 	STX_ATOMIC_W:
-		switch (insn->imm) {
+		switch (IMM) {
 		case BPF_ADD:
 			/* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
 			atomic_add((u32) SRC, (atomic_t *)(unsigned long)
 				   (DST + insn->off));
+			break;
+		case BPF_ADD | BPF_FETCH:
+			SRC = (u32) atomic_fetch_add(
+				(u32) SRC,
+				(atomic_t *)(unsigned long) (DST + insn->off));
+			break;
 		default:
 			goto default_label;
 		}
 		CONT;
+
 	STX_ATOMIC_DW:
-		switch (insn->imm) {
+		switch (IMM) {
 		case BPF_ADD:
 			/* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
 			atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
 				     (DST + insn->off));
+			break;
+		case BPF_ADD | BPF_FETCH:
+			SRC = (u64) atomic64_fetch_add(
+				(u64) SRC,
+				(atomic64_t *)(s64) (DST + insn->off));
+			break;
 		default:
 			goto default_label;
 		}
diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c
index 37c8d6e9b4cc..669cef265493 100644
--- a/kernel/bpf/disasm.c
+++ b/kernel/bpf/disasm.c
@@ -160,6 +160,12 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
 				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
 				insn->dst_reg, insn->off,
 				insn->src_reg);
+		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
+			   insn->imm == (BPF_ADD | BPF_FETCH)) {
+			verbose(cbs->private_data, "(%02x) r%d = atomic_fetch_add(*(%s *)(r%d %+d), r%d)\n",
+				insn->code, insn->src_reg,
+				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
+				insn->dst_reg, insn->off, insn->src_reg);
 		} else {
 			verbose(cbs->private_data, "BUG_%02x\n", insn->code);
 		}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 609cc5e9571f..14f5053daf22 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3600,9 +3600,14 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 
 static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
 {
+	struct bpf_reg_state *regs = cur_regs(env);
 	int err;
 
-	if (insn->imm != BPF_ADD) {
+	switch (insn->imm) {
+	case BPF_ADD:
+	case BPF_ADD | BPF_FETCH:
+		break;
+	default:
 		verbose(env, "BPF_ATOMIC uses invalid atomic opcode %02x\n", insn->imm);
 		return -EINVAL;
 	}
@@ -3631,7 +3636,7 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 	    is_pkt_reg(env, insn->dst_reg) ||
 	    is_flow_key_reg(env, insn->dst_reg) ||
 	    is_sk_reg(env, insn->dst_reg)) {
-		verbose(env, "atomic stores into R%d %s is not allowed\n",
+		verbose(env, "BPF_ATOMIC stores into R%d %s is not allowed\n",
 			insn->dst_reg,
 			reg_type_str[reg_state(env, insn->dst_reg)->type]);
 		return -EACCES;
@@ -3644,8 +3649,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 		return err;
 
 	/* check whether we can write into the same memory */
-	return check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
-				BPF_SIZE(insn->code), BPF_WRITE, -1, true);
+	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
+			       BPF_SIZE(insn->code), BPF_WRITE, -1, true);
+	if (err)
+		return err;
+
+	if (!(insn->imm & BPF_FETCH))
+		return 0;
+
+	/* check and record load of old value into src reg  */
+	err = check_reg_arg(env, insn->src_reg, DST_OP);
+	if (err)
+		return err;
+	regs[insn->src_reg].type = SCALAR_VALUE;
+
+	return 0;
 }
 
 static int __check_stack_boundary(struct bpf_verifier_env *env, u32 regno,
@@ -9490,12 +9508,6 @@ static int do_check(struct bpf_verifier_env *env)
 		} else if (class == BPF_STX) {
 			enum bpf_reg_type *prev_dst_type, dst_reg_type;
 
-			if (((BPF_MODE(insn->code) != BPF_MEM &&
-			      BPF_MODE(insn->code) != BPF_ATOMIC) || insn->imm != 0)) {
-				verbose(env, "BPF_STX uses reserved fields\n");
-				return -EINVAL;
-			}
-
 			if (BPF_MODE(insn->code) == BPF_ATOMIC) {
 				err = check_atomic(env, env->insn_idx, insn);
 				if (err)
@@ -9504,6 +9516,11 @@ static int do_check(struct bpf_verifier_env *env)
 				continue;
 			}
 
+			if (BPF_MODE(insn->code) != BPF_MEM && insn->imm != 0) {
+				verbose(env, "BPF_STX uses reserved fields\n");
+				return -EINVAL;
+			}
+
 			/* check src1 operand */
 			err = check_reg_arg(env, insn->src_reg, SRC_OP);
 			if (err)
diff --git a/tools/include/linux/filter.h b/tools/include/linux/filter.h
index 95ff51d97f25..8f2707ebab18 100644
--- a/tools/include/linux/filter.h
+++ b/tools/include/linux/filter.h
@@ -180,7 +180,15 @@
 		.imm   = BPF_ADD })
 #define BPF_STX_XADD BPF_ATOMIC_ADD /* alias */
 
-/* Memory store, *(uint *) (dst_reg + off16) = imm32 */
+/* Atomic memory add with fetch, src_reg = sync_fetch_and_add(*(dst_reg + off), src_reg); */
+
+#define BPF_ATOMIC_FETCH_ADD(SIZE, DST, SRC, OFF)		\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = BPF_ADD | BPF_FETCH })
 
 #define BPF_ST_MEM(SIZE, DST, OFF, IMM)				\
 	((struct bpf_insn) {					\
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index dcd08783647d..ec7f415f331b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -44,6 +44,9 @@
 #define BPF_CALL	0x80	/* function call */
 #define BPF_EXIT	0x90	/* function return */
 
+/* atomic op type fields (stored in immediate) */
+#define BPF_FETCH	0x01	/* fetch previous value into src reg */
+
 /* Register numbers */
 enum {
 	BPF_REG_0 = 0,
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends
  2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
                   ` (4 preceding siblings ...)
  2020-11-23 17:32 ` [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
@ 2020-11-23 17:32 ` Brendan Jackman
  2020-11-23 19:29   ` Brendan Jackman
  2020-11-24  6:40   ` Alexei Starovoitov
  2020-11-23 17:32 ` [PATCH 7/7] bpf: Add tests for new BPF atomic operations Brendan Jackman
  2020-11-23 17:36 ` [PATCH 0/7] Atomics for eBPF Brendan Jackman
  7 siblings, 2 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:32 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest, Brendan Jackman

These are the operations that implement atomic exchange and
compare-exchange.

They are peculiarly named because of the presence of the separate
FETCH field that tells you whether the instruction writes the value
back to the src register. Neither operation is supported without
BPF_FETCH:

- BPF_CMPSET without BPF_FETCH (i.e. an atomic compare-and-set
  without knowing whether the write was successfully) isn't implemented
  by the kernel, x86, or ARM. It would be a burden on the JIT and it's
  hard to imagine a use for this operation, so it's not supported.

- BPF_SET without BPF_FETCH would be bpf_set, which has pretty
  limited use: all it really lets you do is atomically set 64-bit
  values on 32-bit CPUs. It doesn't imply any barriers.

There are two significant design decisions made for the CMPSET
instruction:

 - To solve the issue that this operation fundamentally has 3
   operands, but we only have two register fields. Therefore the
   operand we compare against (the kernel's API calls it 'old') is
   hard-coded to be R0. x86 has similar design (and A64 doesn't
   have this problem).

   A potential alternative might be to encode the other operand's
   register number in the immediate field.

 - The kernel's atomic_cmpxchg returns the old value, while the C11
   userspace APIs return a boolean indicating the comparison
   result. Which should BPF do? A64 returns the old value. x86 returns
   the old value in the hard-coded register (and also sets a
   flag). That means return-old-value is easier to JIT.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 arch/x86/net/bpf_jit_comp.c    | 29 ++++++++++++++++++++++++-----
 include/linux/filter.h         | 30 ++++++++++++++++++++++++++++++
 include/uapi/linux/bpf.h       |  3 +++
 kernel/bpf/core.c              | 20 ++++++++++++++++++++
 kernel/bpf/disasm.c            | 13 +++++++++++++
 kernel/bpf/verifier.c          | 22 +++++++++++++++++++---
 tools/include/linux/filter.h   | 30 ++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  3 +++
 8 files changed, 142 insertions(+), 8 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index b475bf525424..252b556e8abf 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1255,9 +1255,13 @@ st:			if (is_imm8(insn->off))
 
 		case BPF_STX | BPF_ATOMIC | BPF_W:
 		case BPF_STX | BPF_ATOMIC | BPF_DW:
-			if (BPF_OP(insn->imm) != BPF_ADD) {
-				pr_err("bpf_jit: unknown opcode %02x\n", insn->imm);
-				return -EFAULT;
+			if (insn->imm == BPF_SET) {
+				/*
+				 * atomic_set((u32/u64*)(dst_reg + off), src_reg);
+				 * On x86 atomic_set is just WRITE_ONCE.
+				 */
+				emit_stx(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
+				break;
 			}
 
 			EMIT1(0xF0); /* lock prefix */
@@ -1266,15 +1270,30 @@ st:			if (is_imm8(insn->off))
 				       BPF_SIZE(insn->code) == BPF_DW);
 
 			/* emit opcode */
-			if (insn->imm & BPF_FETCH) {
+			switch (insn->imm) {
+			case BPF_SET | BPF_FETCH:
+				/* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
+				EMIT1(0x87);
+				break;
+			case BPF_CMPSET | BPF_FETCH:
+				/* r0 = atomic_cmpxchg(*(u32/u64*)(dst_reg + off), r0, src_reg); */
+				EMIT2(0x0F, 0xB1);
+				break;
+			case BPF_ADD | BPF_FETCH:
 				/* src_reg = sync_fetch_and_add(*(dst_reg + off), src_reg); */
 				EMIT2(0x0F, 0xC1);
-			} else {
+				break;
+			case BPF_ADD:
 				/* lock *(u32/u64*)(dst_reg + off) += src_reg */
 				EMIT1(0x01);
+				break;
+			default:
+				pr_err("bpf_jit: unknown atomic opcode %02x\n", insn->imm);
+				return -EFAULT;
 			}
 
 			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
+
 			break;
 
 			/* call */
diff --git a/include/linux/filter.h b/include/linux/filter.h
index bf0ff3649f46..402a47fa2276 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -280,6 +280,36 @@ static inline bool insn_is_zext(const struct bpf_insn *insn)
 		.off   = OFF,					\
 		.imm   = BPF_ADD | BPF_FETCH })
 
+/* Atomic exchange, src_reg = atomic_xchg((dst_reg + off), src_reg) */
+
+#define BPF_ATOMIC_XCHG(SIZE, DST, SRC, OFF)			\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = BPF_SET | BPF_FETCH  })
+
+/* Atomic compare-exchange, r0 = atomic_cmpxchg((dst_reg + off), r0, src_reg) */
+
+#define BPF_ATOMIC_CMPXCHG(SIZE, DST, SRC, OFF)			\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = BPF_CMPSET | BPF_FETCH })
+
+/* Atomic set, atomic_set((dst_reg + off), src_reg) */
+
+#define BPF_ATOMIC_SET(SIZE, DST, SRC, OFF)			\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = BPF_SET })
+
 /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
 
 #define BPF_ST_MEM(SIZE, DST, OFF, IMM)				\
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index ec7f415f331b..6996c1856f53 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -45,6 +45,9 @@
 #define BPF_EXIT	0x90	/* function return */
 
 /* atomic op type fields (stored in immediate) */
+#define BPF_SET		0xe0	/* atomic write */
+#define BPF_CMPSET	0xf0	/* atomic compare-and-write */
+
 #define BPF_FETCH	0x01	/* fetch previous value into src reg */
 
 /* Register numbers */
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 49a2a533db60..a549ac2d7651 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1638,6 +1638,16 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 				(u32) SRC,
 				(atomic_t *)(unsigned long) (DST + insn->off));
 			break;
+		case BPF_SET | BPF_FETCH:
+			SRC = (u32) atomic_xchg(
+				(atomic_t *)(unsigned long) (DST + insn->off),
+				(u32) SRC);
+			break;
+		case BPF_CMPSET | BPF_FETCH:
+			BPF_R0 = (u32) atomic_cmpxchg(
+				(atomic_t *)(unsigned long) (DST + insn->off),
+				(u32) BPF_R0, (u32) SRC);
+			break;
 		default:
 			goto default_label;
 		}
@@ -1655,6 +1665,16 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 				(u64) SRC,
 				(atomic64_t *)(s64) (DST + insn->off));
 			break;
+		case BPF_SET | BPF_FETCH:
+			SRC = (u64) atomic64_xchg(
+				(atomic64_t *)(u64) (DST + insn->off),
+				(u64) SRC);
+			break;
+		case BPF_CMPSET | BPF_FETCH:
+			BPF_R0 = (u64) atomic64_cmpxchg(
+				(atomic64_t *)(u64) (DST + insn->off),
+				(u64) BPF_R0, (u64) SRC);
+			break;
 		default:
 			goto default_label;
 		}
diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c
index 669cef265493..7e4a5bfb4e67 100644
--- a/kernel/bpf/disasm.c
+++ b/kernel/bpf/disasm.c
@@ -166,6 +166,19 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
 				insn->code, insn->src_reg,
 				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
 				insn->dst_reg, insn->off, insn->src_reg);
+		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
+			   insn->imm == (BPF_CMPSET | BPF_FETCH)) {
+			verbose(cbs->private_data, "(%02x) r0 = atomic_cmpxchg(*(%s *)(r%d %+d), r0, r%d)\n",
+				insn->code,
+				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
+				insn->dst_reg, insn->off,
+				insn->src_reg);
+		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
+			   insn->imm == (BPF_SET | BPF_FETCH)) {
+			verbose(cbs->private_data, "(%02x) r%d = atomic_xchg(*(%s *)(r%d %+d), r%d)\n",
+				insn->code, insn->src_reg,
+				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
+				insn->dst_reg, insn->off, insn->src_reg);
 		} else {
 			verbose(cbs->private_data, "BUG_%02x\n", insn->code);
 		}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 14f5053daf22..2e611d3695bf 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3602,10 +3602,14 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 {
 	struct bpf_reg_state *regs = cur_regs(env);
 	int err;
+	int load_reg;
 
 	switch (insn->imm) {
 	case BPF_ADD:
 	case BPF_ADD | BPF_FETCH:
+	case BPF_SET:
+	case BPF_SET | BPF_FETCH:
+	case BPF_CMPSET | BPF_FETCH: /* CMPSET without FETCH is not supported */
 		break;
 	default:
 		verbose(env, "BPF_ATOMIC uses invalid atomic opcode %02x\n", insn->imm);
@@ -3627,6 +3631,13 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 	if (err)
 		return err;
 
+	if (BPF_OP(insn->imm) == BPF_CMPSET) {
+		/* check src3 operand */
+		err = check_reg_arg(env, BPF_REG_0, SRC_OP);
+		if (err)
+			return err;
+	}
+
 	if (is_pointer_value(env, insn->src_reg)) {
 		verbose(env, "R%d leaks addr into mem\n", insn->src_reg);
 		return -EACCES;
@@ -3657,11 +3668,16 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 	if (!(insn->imm & BPF_FETCH))
 		return 0;
 
-	/* check and record load of old value into src reg  */
-	err = check_reg_arg(env, insn->src_reg, DST_OP);
+	if (BPF_OP(insn->imm) == BPF_CMPSET)
+		load_reg = BPF_REG_0;
+	else
+		load_reg = insn->src_reg;
+
+	/* check and record load of old value */
+	err = check_reg_arg(env, load_reg, DST_OP);
 	if (err)
 		return err;
-	regs[insn->src_reg].type = SCALAR_VALUE;
+	regs[load_reg].type = SCALAR_VALUE;
 
 	return 0;
 }
diff --git a/tools/include/linux/filter.h b/tools/include/linux/filter.h
index 8f2707ebab18..5a5e4c39c639 100644
--- a/tools/include/linux/filter.h
+++ b/tools/include/linux/filter.h
@@ -190,6 +190,36 @@
 		.off   = OFF,					\
 		.imm   = BPF_ADD | BPF_FETCH })
 
+/* Atomic exchange, src_reg = atomic_xchg((dst_reg + off), src_reg) */
+
+#define BPF_ATOMIC_XCHG(SIZE, DST, SRC, OFF)			\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = BPF_SET | BPF_FETCH })
+
+/* Atomic compare-exchange, r0 = atomic_cmpxchg((dst_reg + off), r0, src_reg) */
+
+#define BPF_ATOMIC_CMPXCHG(SIZE, DST, SRC, OFF)			\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = BPF_CMPSET | BPF_FETCH })
+
+/* Atomic set, atomic_set((dst_reg + off), src_reg) */
+
+#define BPF_ATOMIC_SET(SIZE, DST, SRC, OFF)			\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = BPF_SET })
+
 #define BPF_ST_MEM(SIZE, DST, OFF, IMM)				\
 	((struct bpf_insn) {					\
 		.code  = BPF_ST | BPF_SIZE(SIZE) | BPF_MEM,	\
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index ec7f415f331b..6996c1856f53 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -45,6 +45,9 @@
 #define BPF_EXIT	0x90	/* function return */
 
 /* atomic op type fields (stored in immediate) */
+#define BPF_SET		0xe0	/* atomic write */
+#define BPF_CMPSET	0xf0	/* atomic compare-and-write */
+
 #define BPF_FETCH	0x01	/* fetch previous value into src reg */
 
 /* Register numbers */
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 7/7] bpf: Add tests for new BPF atomic operations
  2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
                   ` (5 preceding siblings ...)
  2020-11-23 17:32 ` [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends Brendan Jackman
@ 2020-11-23 17:32 ` Brendan Jackman
  2020-11-24  0:26   ` Yonghong Song
  2020-11-23 17:36 ` [PATCH 0/7] Atomics for eBPF Brendan Jackman
  7 siblings, 1 reply; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:32 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest, Brendan Jackman

This relies on the work done by Yonghong Song in
https://reviews.llvm.org/D72184

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/bpf/Makefile          |   2 +-
 .../selftests/bpf/prog_tests/atomics_test.c   | 145 ++++++++++++++++++
 .../selftests/bpf/progs/atomics_test.c        |  61 ++++++++
 .../selftests/bpf/verifier/atomic_cmpxchg.c   |  96 ++++++++++++
 .../selftests/bpf/verifier/atomic_fetch_add.c | 106 +++++++++++++
 .../selftests/bpf/verifier/atomic_xchg.c      | 113 ++++++++++++++
 6 files changed, 522 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/atomics_test.c
 create mode 100644 tools/testing/selftests/bpf/progs/atomics_test.c
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_fetch_add.c
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_xchg.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 3d5940cd110d..4e28640ca2d8 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -250,7 +250,7 @@ define CLANG_BPF_BUILD_RULE
 	$(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2)
 	$(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm			\
 		-c $1 -o - || echo "BPF obj compilation failed") | 	\
-	$(LLC) -mattr=dwarfris -march=bpf -mcpu=v3 $4 -filetype=obj -o $2
+	$(LLC) -mattr=dwarfris -march=bpf -mcpu=v4 $4 -filetype=obj -o $2
 endef
 # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
 define CLANG_NOALU32_BPF_BUILD_RULE
diff --git a/tools/testing/selftests/bpf/prog_tests/atomics_test.c b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
new file mode 100644
index 000000000000..a4859d88fc11
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+
+#include "atomics_test.skel.h"
+
+static void test_add(void)
+{
+	struct atomics_test *atomics_skel = NULL;
+	int err, prog_fd;
+	__u32 duration = 0, retval;
+
+	atomics_skel = atomics_test__open_and_load();
+	if (CHECK(!atomics_skel, "atomics_skel_load", "atomics skeleton failed\n"))
+		goto cleanup;
+
+	err = atomics_test__attach(atomics_skel);
+	if (CHECK(err, "atomics_attach", "atomics attach failed: %d\n", err))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(atomics_skel->progs.add);
+	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
+				NULL, NULL, &retval, &duration);
+	if (CHECK(err || retval, "test_run add",
+		  "err %d errno %d retval %d duration %d\n",
+		  err, errno, retval, duration))
+		goto cleanup;
+
+	CHECK(atomics_skel->data->add64_value != 3, "add64_value",
+	      "64bit atomic add value was not incremented (got %lld want 2)\n",
+	      atomics_skel->data->add64_value);
+	CHECK(atomics_skel->bss->add64_result != 1, "add64_result",
+	      "64bit atomic add bad return value (got %lld want 1)\n",
+	      atomics_skel->bss->add64_result);
+
+	CHECK(atomics_skel->data->add32_value != 3, "add32_value",
+	      "32bit atomic add value was not incremented (got %d want 2)\n",
+	      atomics_skel->data->add32_value);
+	CHECK(atomics_skel->bss->add32_result != 1, "add32_result",
+	      "32bit atomic add bad return value (got %d want 1)\n",
+	      atomics_skel->bss->add32_result);
+
+	CHECK(atomics_skel->bss->add_stack_value_copy != 3, "add_stack_value",
+	      "_stackbit atomic add value was not incremented (got %lld want 2)\n",
+	      atomics_skel->bss->add_stack_value_copy);
+	CHECK(atomics_skel->bss->add_stack_result != 1, "add_stack_result",
+	      "_stackbit atomic add bad return value (got %lld want 1)\n",
+	      atomics_skel->bss->add_stack_result);
+
+cleanup:
+	atomics_test__destroy(atomics_skel);
+}
+
+static void test_cmpxchg(void)
+{
+	struct atomics_test *atomics_skel = NULL;
+	int err, prog_fd;
+	__u32 duration = 0, retval;
+
+	atomics_skel = atomics_test__open_and_load();
+	if (CHECK(!atomics_skel, "atomics_skel_load", "atomics skeleton failed\n"))
+		goto cleanup;
+
+	err = atomics_test__attach(atomics_skel);
+	if (CHECK(err, "atomics_attach", "atomics attach failed: %d\n", err))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(atomics_skel->progs.add);
+	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
+				NULL, NULL, &retval, &duration);
+	if (CHECK(err || retval, "test_run add",
+		  "err %d errno %d retval %d duration %d\n",
+		  err, errno, retval, duration))
+		goto cleanup;
+
+	CHECK(atomics_skel->data->cmpxchg64_value != 2, "cmpxchg64_value",
+	      "64bit cmpxchg left unexpected value (got %lld want 2)\n",
+	      atomics_skel->data->cmpxchg64_value);
+	CHECK(atomics_skel->bss->cmpxchg64_result_fail != 1, "cmpxchg_result_fail",
+	      "64bit cmpxchg returned bad result (got %lld want 1)\n",
+	      atomics_skel->bss->cmpxchg64_result_fail);
+	CHECK(atomics_skel->bss->cmpxchg64_result_succeed != 1, "cmpxchg_result_succeed",
+	      "64bit cmpxchg returned bad result (got %lld want 1)\n",
+	      atomics_skel->bss->cmpxchg64_result_succeed);
+
+	CHECK(atomics_skel->data->cmpxchg32_value != 2, "cmpxchg32_value",
+	      "32bit cmpxchg left unexpected value (got %d want 2)\n",
+	      atomics_skel->data->cmpxchg32_value);
+	CHECK(atomics_skel->bss->cmpxchg32_result_fail != 1, "cmpxchg_result_fail",
+	      "32bit cmpxchg returned bad result (got %d want 1)\n",
+	      atomics_skel->bss->cmpxchg32_result_fail);
+	CHECK(atomics_skel->bss->cmpxchg32_result_succeed != 1, "cmpxchg_result_succeed",
+	      "32bit cmpxchg returned bad result (got %d want 1)\n",
+	      atomics_skel->bss->cmpxchg32_result_succeed);
+
+cleanup:
+	atomics_test__destroy(atomics_skel);
+}
+
+static void test_xchg(void)
+{
+	struct atomics_test *atomics_skel = NULL;
+	int err, prog_fd;
+	__u32 duration = 0, retval;
+
+	atomics_skel = atomics_test__open_and_load();
+	if (CHECK(!atomics_skel, "atomics_skel_load", "atomics skeleton failed\n"))
+		goto cleanup;
+
+	err = atomics_test__attach(atomics_skel);
+	if (CHECK(err, "atomics_attach", "atomics attach failed: %d\n", err))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(atomics_skel->progs.add);
+	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
+				NULL, NULL, &retval, &duration);
+	if (CHECK(err || retval, "test_run add",
+		  "err %d errno %d retval %d duration %d\n",
+		  err, errno, retval, duration))
+		goto cleanup;
+
+	CHECK(atomics_skel->data->xchg64_value != 2, "xchg64_value",
+	      "64bit xchg left unexpected value (got %lld want 2)\n",
+	      atomics_skel->data->xchg64_value);
+	CHECK(atomics_skel->bss->xchg64_result != 1, "xchg_result",
+	      "64bit xchg returned bad result (got %lld want 1)\n",
+	      atomics_skel->bss->xchg64_result);
+
+	CHECK(atomics_skel->data->xchg32_value != 2, "xchg32_value",
+	      "32bit xchg left unexpected value (got %d want 2)\n",
+	      atomics_skel->data->xchg32_value);
+	CHECK(atomics_skel->bss->xchg32_result != 1, "xchg_result",
+	      "32bit xchg returned bad result (got %d want 1)\n",
+	      atomics_skel->bss->xchg32_result);
+
+cleanup:
+	atomics_test__destroy(atomics_skel);
+}
+
+void test_atomics_test(void)
+{
+	test_add();
+	test_cmpxchg();
+	test_xchg();
+}
diff --git a/tools/testing/selftests/bpf/progs/atomics_test.c b/tools/testing/selftests/bpf/progs/atomics_test.c
new file mode 100644
index 000000000000..d81f45eb6c45
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/atomics_test.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+__u64 add64_value = 1;
+__u64 add64_result;
+__u32 add32_value = 1;
+__u32 add32_result;
+__u64 add_stack_value_copy;
+__u64 add_stack_result;
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(add, int a)
+{
+	__u64 add_stack_value = 1;
+
+	add64_result = __sync_fetch_and_add(&add64_value, 2);
+	add32_result = __sync_fetch_and_add(&add32_value, 2);
+	add_stack_result = __sync_fetch_and_add(&add_stack_value, 2);
+	add_stack_value_copy = add_stack_value;
+
+	return 0;
+}
+
+__u64 cmpxchg64_value = 1;
+__u64 cmpxchg64_result_fail;
+__u64 cmpxchg64_result_succeed;
+__u32 cmpxchg32_value = 1;
+__u32 cmpxchg32_result_fail;
+__u32 cmpxchg32_result_succeed;
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(cmpxchg, int a)
+{
+	cmpxchg64_result_fail = __sync_val_compare_and_swap(
+		&cmpxchg64_value, 0, 3);
+	cmpxchg64_result_succeed = __sync_val_compare_and_swap(
+		&cmpxchg64_value, 1, 2);
+
+	cmpxchg32_result_fail = __sync_val_compare_and_swap(
+		&cmpxchg32_value, 0, 3);
+	cmpxchg32_result_succeed = __sync_val_compare_and_swap(
+		&cmpxchg32_value, 1, 2);
+
+	return 0;
+}
+
+__u64 xchg64_value = 1;
+__u64 xchg64_result;
+__u32 xchg32_value = 1;
+__u32 xchg32_result;
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(xchg, int a)
+{
+	__u64 val64 = 2;
+	__u32 val32 = 2;
+
+	__atomic_exchange(&xchg64_value, &val64, &xchg64_result, __ATOMIC_RELAXED);
+	__atomic_exchange(&xchg32_value, &val32, &xchg32_result, __ATOMIC_RELAXED);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c b/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
new file mode 100644
index 000000000000..eb43a06428fa
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
@@ -0,0 +1,96 @@
+{
+	"atomic compare-and-exchange smoketest - 64bit",
+	.insns = {
+	/* val = 3; */
+	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 3),
+	/* old = atomic_cmpxchg(&val, 2, 4); */
+	BPF_MOV64_IMM(BPF_REG_1, 4),
+	BPF_MOV64_IMM(BPF_REG_0, 2),
+	BPF_ATOMIC_CMPXCHG(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
+	/* if (old != 3) exit(2); */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 3, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	/* if (val != 3) exit(3); */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 3, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 3),
+	BPF_EXIT_INSN(),
+	/* old = atomic_cmpxchg(&val, 3, 4); */
+	BPF_MOV64_IMM(BPF_REG_1, 4),
+	BPF_MOV64_IMM(BPF_REG_0, 3),
+	BPF_ATOMIC_CMPXCHG(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
+	/* if (old != 3) exit(4); */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 3, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 4),
+	BPF_EXIT_INSN(),
+	/* if (val != 4) exit(5); */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 4, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 5),
+	BPF_EXIT_INSN(),
+	/* exit(0); */
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"atomic compare-and-exchange smoketest - 32bit",
+	.insns = {
+	/* val = 3; */
+	BPF_ST_MEM(BPF_W, BPF_REG_10, -4, 3),
+	/* old = atomic_cmpxchg(&val, 2, 4); */
+	BPF_MOV32_IMM(BPF_REG_1, 4),
+	BPF_MOV32_IMM(BPF_REG_0, 2),
+	BPF_ATOMIC_CMPXCHG(BPF_W, BPF_REG_10, BPF_REG_1, -4),
+	/* if (old != 3) exit(2); */
+	BPF_JMP32_IMM(BPF_JEQ, BPF_REG_0, 3, 2),
+	BPF_MOV32_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	/* if (val != 3) exit(3); */
+	BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_10, -4),
+	BPF_JMP32_IMM(BPF_JEQ, BPF_REG_0, 3, 2),
+	BPF_MOV32_IMM(BPF_REG_0, 3),
+	BPF_EXIT_INSN(),
+	/* old = atomic_cmpxchg(&val, 3, 4); */
+	BPF_MOV32_IMM(BPF_REG_1, 4),
+	BPF_MOV32_IMM(BPF_REG_0, 3),
+	BPF_ATOMIC_CMPXCHG(BPF_W, BPF_REG_10, BPF_REG_1, -4),
+	/* if (old != 3) exit(4); */
+	BPF_JMP32_IMM(BPF_JEQ, BPF_REG_0, 3, 2),
+	BPF_MOV32_IMM(BPF_REG_0, 4),
+	BPF_EXIT_INSN(),
+	/* if (val != 4) exit(5); */
+	BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_10, -4),
+	BPF_JMP32_IMM(BPF_JEQ, BPF_REG_0, 4, 2),
+	BPF_MOV32_IMM(BPF_REG_0, 5),
+	BPF_EXIT_INSN(),
+	/* exit(0); */
+	BPF_MOV32_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"Can't use cmpxchg on uninit src reg",
+	.insns = {
+	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 3),
+	BPF_MOV64_IMM(BPF_REG_0, 3),
+	BPF_ATOMIC_CMPXCHG(BPF_DW, BPF_REG_10, BPF_REG_2, -8),
+	BPF_EXIT_INSN(),
+	},
+	.result = REJECT,
+	.errstr = "!read_ok",
+},
+{
+	"Can't use cmpxchg on uninit memory",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_0, 3),
+	BPF_MOV64_IMM(BPF_REG_2, 4),
+	BPF_ATOMIC_CMPXCHG(BPF_DW, BPF_REG_10, BPF_REG_2, -8),
+	BPF_EXIT_INSN(),
+	},
+	.result = REJECT,
+	.errstr = "invalid read from stack",
+},
diff --git a/tools/testing/selftests/bpf/verifier/atomic_fetch_add.c b/tools/testing/selftests/bpf/verifier/atomic_fetch_add.c
new file mode 100644
index 000000000000..c3236510cb64
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/atomic_fetch_add.c
@@ -0,0 +1,106 @@
+{
+	"BPF_ATOMIC_FETCH_ADD smoketest - 64bit",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	/* Write 3 to stack */
+	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 3),
+	/* Put a 1 in R1, add it to the 3 on the stack, and load the value back into R1 */
+	BPF_MOV64_IMM(BPF_REG_1, 1),
+	BPF_ATOMIC_FETCH_ADD(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
+	/* Check the value we loaded back was 3 */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 3, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 1),
+	BPF_EXIT_INSN(),
+	/* Load value from stack */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_10, -8),
+	/* Check value loaded from stack was 4 */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 4, 1),
+	BPF_MOV64_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"BPF_ATOMIC_FETCH_ADD smoketest - 32bit",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	/* Write 3 to stack */
+	BPF_ST_MEM(BPF_W, BPF_REG_10, -4, 3),
+	/* Put a 1 in R1, add it to the 3 on the stack, and load the value back into R1 */
+	BPF_MOV32_IMM(BPF_REG_1, 1),
+	BPF_ATOMIC_FETCH_ADD(BPF_W, BPF_REG_10, BPF_REG_1, -4),
+	/* Check the value we loaded back was 3 */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 3, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 1),
+	BPF_EXIT_INSN(),
+	/* Load value from stack */
+	BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_10, -4),
+	/* Check value loaded from stack was 4 */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 4, 1),
+	BPF_MOV64_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"Can't use ATM_FETCH_ADD on frame pointer",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 3),
+		BPF_ATOMIC_FETCH_ADD(BPF_DW, BPF_REG_10, BPF_REG_10, -8),
+		BPF_EXIT_INSN(),
+	},
+	.result = REJECT,
+	.errstr_unpriv = "R10 leaks addr into mem",
+	.errstr = "frame pointer is read only",
+},
+{
+	"Can't use ATM_FETCH_ADD on uninit src reg",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 3),
+		BPF_ATOMIC_FETCH_ADD(BPF_DW, BPF_REG_10, BPF_REG_2, -8),
+		BPF_EXIT_INSN(),
+	},
+	.result = REJECT,
+	/* It happens that the address leak check is first, but it would also be
+	 * complain about the fact that we're trying to modify R10.
+	 */
+	.errstr = "!read_ok",
+},
+{
+	"Can't use ATM_FETCH_ADD on uninit dst reg",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_ATOMIC_FETCH_ADD(BPF_DW, BPF_REG_2, BPF_REG_0, -8),
+		BPF_EXIT_INSN(),
+	},
+	.result = REJECT,
+	/* It happens that the address leak check is first, but it would also be
+	 * complain about the fact that we're trying to modify R10.
+	 */
+	.errstr = "!read_ok",
+},
+{
+	"Can't use ATM_FETCH_ADD on kernel memory",
+	.insns = {
+		/* This is an fentry prog, context is array of the args of the
+		 * kernel function being called. Load first arg into R2.
+		 */
+		BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_1, 0),
+		/* First arg of bpf_fentry_test7 is a pointer to a struct.
+		 * Attempt to modify that struct. Verifier shouldn't let us
+		 * because it's kernel memory.
+		 */
+		BPF_MOV64_IMM(BPF_REG_3, 1),
+		BPF_ATOMIC_FETCH_ADD(BPF_DW, BPF_REG_2, BPF_REG_3, 0),
+		/* Done */
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACING,
+	.expected_attach_type = BPF_TRACE_FENTRY,
+	.kfunc = "bpf_fentry_test7",
+	.result = REJECT,
+	.errstr = "only read is supported",
+},
diff --git a/tools/testing/selftests/bpf/verifier/atomic_xchg.c b/tools/testing/selftests/bpf/verifier/atomic_xchg.c
new file mode 100644
index 000000000000..b39d8c0dabf9
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/atomic_xchg.c
@@ -0,0 +1,113 @@
+{
+	"atomic exchange smoketest - 64bit",
+	.insns = {
+	/* val = 3; */
+	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 3),
+	/* old = atomic_xchg(&val, 4); */
+	BPF_MOV64_IMM(BPF_REG_1, 4),
+	BPF_ATOMIC_XCHG(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
+	/* if (old != 3) exit(1); */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 3, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 1),
+	BPF_EXIT_INSN(),
+	/* if (val != 4) exit(2); */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 4, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	/* exit(0); */
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"atomic exchange smoketest - 32bit",
+	.insns = {
+	/* val = 3; */
+	BPF_ST_MEM(BPF_W, BPF_REG_10, -4, 3),
+	/* old = atomic_xchg(&val, 4); */
+	BPF_MOV32_IMM(BPF_REG_1, 4),
+	BPF_ATOMIC_XCHG(BPF_W, BPF_REG_10, BPF_REG_1, -4),
+	/* if (old != 3) exit(1); */
+	BPF_JMP32_IMM(BPF_JEQ, BPF_REG_1, 3, 2),
+	BPF_MOV32_IMM(BPF_REG_0, 1),
+	BPF_EXIT_INSN(),
+	/* if (val != 4) exit(2); */
+	BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_10, -4),
+	BPF_JMP32_IMM(BPF_JEQ, BPF_REG_0, 4, 2),
+	BPF_MOV32_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	/* exit(0); */
+	BPF_MOV32_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"atomic set smoketest - 64bit",
+	.insns = {
+	/* val = 3; */
+	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 3),
+	/* atomic_xchg(&val, 4); */
+	BPF_MOV64_IMM(BPF_REG_1, 4),
+	BPF_ATOMIC_SET(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
+	/* r1 should not be clobbered, no BPF_FETCH flag */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 4, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 1),
+	BPF_EXIT_INSN(),
+	/* if (val != 4) exit(2); */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 4, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	/* exit(0); */
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"atomic set smoketest - 32bit",
+	.insns = {
+	/* val = 3; */
+	BPF_ST_MEM(BPF_W, BPF_REG_10, -4, 3),
+	/* atomic_xchg(&val, 4); */
+	BPF_MOV32_IMM(BPF_REG_1, 4),
+	BPF_ATOMIC_SET(BPF_W, BPF_REG_10, BPF_REG_1, -4),
+	/* r1 should not be clobbered, no BPF_FETCH flag */
+	BPF_JMP32_IMM(BPF_JEQ, BPF_REG_1, 4, 2),
+	BPF_MOV32_IMM(BPF_REG_0, 1),
+	BPF_EXIT_INSN(),
+	/* if (val != 4) exit(2); */
+	BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_10, -4),
+	BPF_JMP32_IMM(BPF_JEQ, BPF_REG_0, 4, 2),
+	BPF_MOV32_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	/* exit(0); */
+	BPF_MOV32_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"Can't use atomic set on kernel memory",
+	.insns = {
+	/* This is an fentry prog, context is array of the args of the
+	 * kernel function being called. Load first arg into R2.
+	 */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_1, 0),
+	/* First arg of bpf_fentry_test7 is a pointer to a struct.
+	 * Attempt to modify that struct. Verifier shouldn't let us
+	 * because it's kernel memory.
+	 */
+	BPF_MOV64_IMM(BPF_REG_3, 1),
+	BPF_ATOMIC_SET(BPF_DW, BPF_REG_2, BPF_REG_3, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACING,
+	.expected_attach_type = BPF_TRACE_FENTRY,
+	.kfunc = "bpf_fentry_test7",
+	.result = REJECT,
+	.errstr = "only read is supported",
+},
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/7] Atomics for eBPF
  2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
                   ` (6 preceding siblings ...)
  2020-11-23 17:32 ` [PATCH 7/7] bpf: Add tests for new BPF atomic operations Brendan Jackman
@ 2020-11-23 17:36 ` Brendan Jackman
  7 siblings, 0 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 17:36 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest

Agh, sorry - should be [PATCH bpf-next].

These patches apply to commit 91b2db27d3ff

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends
  2020-11-23 17:32 ` [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends Brendan Jackman
@ 2020-11-23 19:29   ` Brendan Jackman
  2020-11-24  6:40   ` Alexei Starovoitov
  1 sibling, 0 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-23 19:29 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, KP Singh,
	Florent Revest

Looks like I half-baked some last-minute changes before sending this
out. If you disable the JIT, test_verifier will fail.

On Mon, Nov 23, 2020 at 05:32:01PM +0000, Brendan Jackman wrote:
> diff --git a/include/linux/filter.h b/include/linux/filter.h
...
> +#define BPF_ATOMIC_SET(SIZE, DST, SRC, OFF)			\
> +	((struct bpf_insn) {					\
> +		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC,	\
> +		.dst_reg = DST,					\
> +		.src_reg = SRC,					\
> +		.off   = OFF,					\
> +		.imm   = BPF_SET })
> +

Should be deleted, and in the tools/ copy too. There's a test case in
the later commit that should be changed to assert that instructions like
this fail to verify.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 14f5053daf22..2e611d3695bf 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3602,10 +3602,14 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>  {
>  	struct bpf_reg_state *regs = cur_regs(env);
>  	int err;
> +	int load_reg;
>  
>  	switch (insn->imm) {
>  	case BPF_ADD:
>  	case BPF_ADD | BPF_FETCH:
> +	case BPF_SET:

Should be deleted.

> +	case BPF_SET | BPF_FETCH:
> +	case BPF_CMPSET | BPF_FETCH: /* CMPSET without FETCH is not supported */
>  		break;
>  	default:
>  		verbose(env, "BPF_ATOMIC uses invalid atomic opcode %02x\n", insn->imm);

I'll fold the fix into the next revision along with the comments
generated by this initial version.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
  2020-11-23 17:32 ` [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
@ 2020-11-23 21:12     ` kernel test robot
  2020-11-23 21:51     ` kernel test robot
  2020-11-24  6:52   ` Alexei Starovoitov
  2 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-11-23 21:12 UTC (permalink / raw)
  To: Brendan Jackman, bpf
  Cc: kbuild-all, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest, Brendan Jackman

[-- Attachment #1: Type: text/plain, Size: 4058 bytes --]

Hi Brendan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]
[also build test WARNING on bpf/master powerpc/next linus/master v5.10-rc5 next-20201123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brendan-Jackman/Atomics-for-eBPF/20201124-013549
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: arc-randconfig-s031-20201123 (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-134-gb59dbdaf-dirty
        # https://github.com/0day-ci/linux/commit/b2b5923320904ef8c33183e8e88042588eb99397
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brendan-Jackman/Atomics-for-eBPF/20201124-013549
        git checkout b2b5923320904ef8c33183e8e88042588eb99397
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   kernel/bpf/core.c:209:49: sparse: sparse: arithmetics on pointers to functions
   kernel/bpf/core.c:1521:43: sparse: sparse: arithmetics on pointers to functions
   kernel/bpf/core.c:1526:48: sparse: sparse: arithmetics on pointers to functions
   kernel/bpf/core.c:1742:77: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/core.c: note: in included file (through include/trace/trace_events.h, include/trace/define_trace.h, include/trace/events/xdp.h, ...):
   include/trace/events/xdp.h:308:1: sparse: sparse: Using plain integer as NULL pointer
   include/trace/events/xdp.h:335:1: sparse: sparse: Using plain integer as NULL pointer
   include/trace/events/xdp.h:369:1: sparse: sparse: Using plain integer as NULL pointer
>> kernel/bpf/core.c:1656:60: sparse: sparse: non size-preserving integer to pointer cast

vim +1656 kernel/bpf/core.c

  1628	
  1629		STX_ATOMIC_W:
  1630			switch (IMM) {
  1631			case BPF_ADD:
  1632				/* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
  1633				atomic_add((u32) SRC, (atomic_t *)(unsigned long)
  1634					   (DST + insn->off));
  1635				break;
  1636			case BPF_ADD | BPF_FETCH:
  1637				SRC = (u32) atomic_fetch_add(
  1638					(u32) SRC,
  1639					(atomic_t *)(unsigned long) (DST + insn->off));
  1640				break;
  1641			default:
  1642				goto default_label;
  1643			}
  1644			CONT;
  1645	
  1646		STX_ATOMIC_DW:
  1647			switch (IMM) {
  1648			case BPF_ADD:
  1649				/* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
  1650				atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
  1651					     (DST + insn->off));
  1652				break;
  1653			case BPF_ADD | BPF_FETCH:
  1654				SRC = (u64) atomic64_fetch_add(
  1655					(u64) SRC,
> 1656					(atomic64_t *)(s64) (DST + insn->off));
  1657				break;
  1658			default:
  1659				goto default_label;
  1660			}
  1661			CONT;
  1662	
  1663		default_label:
  1664			/* If we ever reach this, we have a bug somewhere. Die hard here
  1665			 * instead of just returning 0; we could be somewhere in a subprog,
  1666			 * so execution could continue otherwise which we do /not/ want.
  1667			 *
  1668			 * Note, verifier whitelists all opcodes in bpf_opcode_in_insntable().
  1669			 */
  1670			pr_warn("BPF interpreter: unknown opcode %02x (imm: 0x%x)\n",
  1671				insn->code, insn->imm);
  1672			BUG_ON(1);
  1673			return 0;
  1674	}
  1675	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31973 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
@ 2020-11-23 21:12     ` kernel test robot
  0 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-11-23 21:12 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 4155 bytes --]

Hi Brendan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]
[also build test WARNING on bpf/master powerpc/next linus/master v5.10-rc5 next-20201123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brendan-Jackman/Atomics-for-eBPF/20201124-013549
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: arc-randconfig-s031-20201123 (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-134-gb59dbdaf-dirty
        # https://github.com/0day-ci/linux/commit/b2b5923320904ef8c33183e8e88042588eb99397
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brendan-Jackman/Atomics-for-eBPF/20201124-013549
        git checkout b2b5923320904ef8c33183e8e88042588eb99397
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   kernel/bpf/core.c:209:49: sparse: sparse: arithmetics on pointers to functions
   kernel/bpf/core.c:1521:43: sparse: sparse: arithmetics on pointers to functions
   kernel/bpf/core.c:1526:48: sparse: sparse: arithmetics on pointers to functions
   kernel/bpf/core.c:1742:77: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/core.c: note: in included file (through include/trace/trace_events.h, include/trace/define_trace.h, include/trace/events/xdp.h, ...):
   include/trace/events/xdp.h:308:1: sparse: sparse: Using plain integer as NULL pointer
   include/trace/events/xdp.h:335:1: sparse: sparse: Using plain integer as NULL pointer
   include/trace/events/xdp.h:369:1: sparse: sparse: Using plain integer as NULL pointer
>> kernel/bpf/core.c:1656:60: sparse: sparse: non size-preserving integer to pointer cast

vim +1656 kernel/bpf/core.c

  1628	
  1629		STX_ATOMIC_W:
  1630			switch (IMM) {
  1631			case BPF_ADD:
  1632				/* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
  1633				atomic_add((u32) SRC, (atomic_t *)(unsigned long)
  1634					   (DST + insn->off));
  1635				break;
  1636			case BPF_ADD | BPF_FETCH:
  1637				SRC = (u32) atomic_fetch_add(
  1638					(u32) SRC,
  1639					(atomic_t *)(unsigned long) (DST + insn->off));
  1640				break;
  1641			default:
  1642				goto default_label;
  1643			}
  1644			CONT;
  1645	
  1646		STX_ATOMIC_DW:
  1647			switch (IMM) {
  1648			case BPF_ADD:
  1649				/* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
  1650				atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
  1651					     (DST + insn->off));
  1652				break;
  1653			case BPF_ADD | BPF_FETCH:
  1654				SRC = (u64) atomic64_fetch_add(
  1655					(u64) SRC,
> 1656					(atomic64_t *)(s64) (DST + insn->off));
  1657				break;
  1658			default:
  1659				goto default_label;
  1660			}
  1661			CONT;
  1662	
  1663		default_label:
  1664			/* If we ever reach this, we have a bug somewhere. Die hard here
  1665			 * instead of just returning 0; we could be somewhere in a subprog,
  1666			 * so execution could continue otherwise which we do /not/ want.
  1667			 *
  1668			 * Note, verifier whitelists all opcodes in bpf_opcode_in_insntable().
  1669			 */
  1670			pr_warn("BPF interpreter: unknown opcode %02x (imm: 0x%x)\n",
  1671				insn->code, insn->imm);
  1672			BUG_ON(1);
  1673			return 0;
  1674	}
  1675	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31973 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
  2020-11-23 17:32 ` [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
@ 2020-11-23 21:51     ` kernel test robot
  2020-11-23 21:51     ` kernel test robot
  2020-11-24  6:52   ` Alexei Starovoitov
  2 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-11-23 21:51 UTC (permalink / raw)
  To: Brendan Jackman, bpf
  Cc: kbuild-all, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest, Brendan Jackman

[-- Attachment #1: Type: text/plain, Size: 6707 bytes --]

Hi Brendan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]
[also build test WARNING on bpf/master powerpc/next linus/master v5.10-rc5 next-20201123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brendan-Jackman/Atomics-for-eBPF/20201124-013549
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: m68k-randconfig-r005-20201123 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b2b5923320904ef8c33183e8e88042588eb99397
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brendan-Jackman/Atomics-for-eBPF/20201124-013549
        git checkout b2b5923320904ef8c33183e8e88042588eb99397
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from arch/m68k/include/asm/irqflags.h:6,
                    from include/linux/irqflags.h:16,
                    from arch/m68k/include/asm/atomic.h:6,
                    from include/linux/atomic.h:7,
                    from include/linux/filter.h:10,
                    from kernel/bpf/core.c:21:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_no.h:33:50: warning: ordered comparison of pointer with null pointer [-Wextra]
      33 | #define virt_addr_valid(kaddr) (((void *)(kaddr) >= (void *)PAGE_OFFSET) && \
         |                                                  ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   kernel/bpf/core.c: At top level:
   kernel/bpf/core.c:1358:12: warning: no previous prototype for 'bpf_probe_read_kernel' [-Wmissing-prototypes]
    1358 | u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
         |            ^~~~~~~~~~~~~~~~~~~~~
   In file included from kernel/bpf/core.c:21:
   kernel/bpf/core.c: In function '___bpf_prog_run':
   include/linux/filter.h:899:3: warning: cast between incompatible function types from 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64,  const struct bpf_insn *)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  const struct bpf_insn *)'} [-Wcast-function-type]
     899 |  ((u64 (*)(u64, u64, u64, u64, u64, const struct bpf_insn *)) \
         |   ^
   kernel/bpf/core.c:1526:13: note: in expansion of macro '__bpf_call_base_args'
    1526 |   BPF_R0 = (__bpf_call_base_args + insn->imm)(BPF_R1, BPF_R2,
         |             ^~~~~~~~~~~~~~~~~~~~
>> kernel/bpf/core.c:1656:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1656 |     (atomic64_t *)(s64) (DST + insn->off));
         |     ^
   In file included from kernel/bpf/core.c:21:
   kernel/bpf/core.c: In function 'bpf_patch_call_args':
   include/linux/filter.h:899:3: warning: cast between incompatible function types from 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64,  const struct bpf_insn *)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  const struct bpf_insn *)'} [-Wcast-function-type]
     899 |  ((u64 (*)(u64, u64, u64, u64, u64, const struct bpf_insn *)) \
         |   ^
   kernel/bpf/core.c:1743:3: note: in expansion of macro '__bpf_call_base_args'
    1743 |   __bpf_call_base_args;
         |   ^~~~~~~~~~~~~~~~~~~~

vim +1656 kernel/bpf/core.c

  1628	
  1629		STX_ATOMIC_W:
  1630			switch (IMM) {
  1631			case BPF_ADD:
  1632				/* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
  1633				atomic_add((u32) SRC, (atomic_t *)(unsigned long)
  1634					   (DST + insn->off));
  1635				break;
  1636			case BPF_ADD | BPF_FETCH:
  1637				SRC = (u32) atomic_fetch_add(
  1638					(u32) SRC,
  1639					(atomic_t *)(unsigned long) (DST + insn->off));
  1640				break;
  1641			default:
  1642				goto default_label;
  1643			}
  1644			CONT;
  1645	
  1646		STX_ATOMIC_DW:
  1647			switch (IMM) {
  1648			case BPF_ADD:
  1649				/* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
  1650				atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
  1651					     (DST + insn->off));
  1652				break;
  1653			case BPF_ADD | BPF_FETCH:
  1654				SRC = (u64) atomic64_fetch_add(
  1655					(u64) SRC,
> 1656					(atomic64_t *)(s64) (DST + insn->off));
  1657				break;
  1658			default:
  1659				goto default_label;
  1660			}
  1661			CONT;
  1662	
  1663		default_label:
  1664			/* If we ever reach this, we have a bug somewhere. Die hard here
  1665			 * instead of just returning 0; we could be somewhere in a subprog,
  1666			 * so execution could continue otherwise which we do /not/ want.
  1667			 *
  1668			 * Note, verifier whitelists all opcodes in bpf_opcode_in_insntable().
  1669			 */
  1670			pr_warn("BPF interpreter: unknown opcode %02x (imm: 0x%x)\n",
  1671				insn->code, insn->imm);
  1672			BUG_ON(1);
  1673			return 0;
  1674	}
  1675	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21919 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
@ 2020-11-23 21:51     ` kernel test robot
  0 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-11-23 21:51 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 6838 bytes --]

Hi Brendan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]
[also build test WARNING on bpf/master powerpc/next linus/master v5.10-rc5 next-20201123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brendan-Jackman/Atomics-for-eBPF/20201124-013549
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: m68k-randconfig-r005-20201123 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b2b5923320904ef8c33183e8e88042588eb99397
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brendan-Jackman/Atomics-for-eBPF/20201124-013549
        git checkout b2b5923320904ef8c33183e8e88042588eb99397
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from arch/m68k/include/asm/irqflags.h:6,
                    from include/linux/irqflags.h:16,
                    from arch/m68k/include/asm/atomic.h:6,
                    from include/linux/atomic.h:7,
                    from include/linux/filter.h:10,
                    from kernel/bpf/core.c:21:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_no.h:33:50: warning: ordered comparison of pointer with null pointer [-Wextra]
      33 | #define virt_addr_valid(kaddr) (((void *)(kaddr) >= (void *)PAGE_OFFSET) && \
         |                                                  ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   kernel/bpf/core.c: At top level:
   kernel/bpf/core.c:1358:12: warning: no previous prototype for 'bpf_probe_read_kernel' [-Wmissing-prototypes]
    1358 | u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
         |            ^~~~~~~~~~~~~~~~~~~~~
   In file included from kernel/bpf/core.c:21:
   kernel/bpf/core.c: In function '___bpf_prog_run':
   include/linux/filter.h:899:3: warning: cast between incompatible function types from 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64,  const struct bpf_insn *)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  const struct bpf_insn *)'} [-Wcast-function-type]
     899 |  ((u64 (*)(u64, u64, u64, u64, u64, const struct bpf_insn *)) \
         |   ^
   kernel/bpf/core.c:1526:13: note: in expansion of macro '__bpf_call_base_args'
    1526 |   BPF_R0 = (__bpf_call_base_args + insn->imm)(BPF_R1, BPF_R2,
         |             ^~~~~~~~~~~~~~~~~~~~
>> kernel/bpf/core.c:1656:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1656 |     (atomic64_t *)(s64) (DST + insn->off));
         |     ^
   In file included from kernel/bpf/core.c:21:
   kernel/bpf/core.c: In function 'bpf_patch_call_args':
   include/linux/filter.h:899:3: warning: cast between incompatible function types from 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64,  const struct bpf_insn *)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  const struct bpf_insn *)'} [-Wcast-function-type]
     899 |  ((u64 (*)(u64, u64, u64, u64, u64, const struct bpf_insn *)) \
         |   ^
   kernel/bpf/core.c:1743:3: note: in expansion of macro '__bpf_call_base_args'
    1743 |   __bpf_call_base_args;
         |   ^~~~~~~~~~~~~~~~~~~~

vim +1656 kernel/bpf/core.c

  1628	
  1629		STX_ATOMIC_W:
  1630			switch (IMM) {
  1631			case BPF_ADD:
  1632				/* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
  1633				atomic_add((u32) SRC, (atomic_t *)(unsigned long)
  1634					   (DST + insn->off));
  1635				break;
  1636			case BPF_ADD | BPF_FETCH:
  1637				SRC = (u32) atomic_fetch_add(
  1638					(u32) SRC,
  1639					(atomic_t *)(unsigned long) (DST + insn->off));
  1640				break;
  1641			default:
  1642				goto default_label;
  1643			}
  1644			CONT;
  1645	
  1646		STX_ATOMIC_DW:
  1647			switch (IMM) {
  1648			case BPF_ADD:
  1649				/* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
  1650				atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
  1651					     (DST + insn->off));
  1652				break;
  1653			case BPF_ADD | BPF_FETCH:
  1654				SRC = (u64) atomic64_fetch_add(
  1655					(u64) SRC,
> 1656					(atomic64_t *)(s64) (DST + insn->off));
  1657				break;
  1658			default:
  1659				goto default_label;
  1660			}
  1661			CONT;
  1662	
  1663		default_label:
  1664			/* If we ever reach this, we have a bug somewhere. Die hard here
  1665			 * instead of just returning 0; we could be somewhere in a subprog,
  1666			 * so execution could continue otherwise which we do /not/ want.
  1667			 *
  1668			 * Note, verifier whitelists all opcodes in bpf_opcode_in_insntable().
  1669			 */
  1670			pr_warn("BPF interpreter: unknown opcode %02x (imm: 0x%x)\n",
  1671				insn->code, insn->imm);
  1672			BUG_ON(1);
  1673			return 0;
  1674	}
  1675	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 21919 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  2020-11-23 17:31 ` [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
@ 2020-11-23 23:54   ` Yonghong Song
  2020-11-24 11:02     ` Brendan Jackman
  2020-11-24  3:28     ` kernel test robot
  2020-11-24  6:50   ` Alexei Starovoitov
  2 siblings, 1 reply; 29+ messages in thread
From: Yonghong Song @ 2020-11-23 23:54 UTC (permalink / raw)
  To: Brendan Jackman, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, KP Singh, Florent Revest



On 11/23/20 9:31 AM, 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           | 27 +++++++++-------
>   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                  | 26 +++++++++-------
>   arch/sparc/net/bpf_jit_comp_64.c              | 14 +++++++--
>   arch/x86/net/bpf_jit_comp.c                   | 30 +++++++++++-------
>   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 | 13 +++++---
>   include/linux/filter.h                        |  8 +++--
>   include/uapi/linux/bpf.h                      |  3 +-
>   kernel/bpf/core.c                             | 31 +++++++++++++------
>   kernel/bpf/disasm.c                           |  6 ++--
>   kernel/bpf/verifier.c                         | 24 ++++++++------
>   lib/test_bpf.c                                |  2 +-
>   samples/bpf/bpf_insn.h                        |  4 +--
>   samples/bpf/sock_example.c                    |  3 +-
>   samples/bpf/test_cgrp2_attach.c               |  6 ++--
>   tools/include/linux/filter.h                  |  7 +++--
>   tools/include/uapi/linux/bpf.h                |  3 +-
>   .../bpf/prog_tests/cgroup_attach_multi.c      |  6 ++--
>   tools/testing/selftests/bpf/verifier/ctx.c    |  6 ++--
>   .../testing/selftests/bpf/verifier/leak_ptr.c |  4 +--
>   tools/testing/selftests/bpf/verifier/unpriv.c |  3 +-
>   tools/testing/selftests/bpf/verifier/xadd.c   |  2 +-
>   30 files changed, 230 insertions(+), 117 deletions(-)
> 
> diff --git a/Documentation/networking/filter.rst b/Documentation/networking/filter.rst
> index debb59e374de..a9847662bbab 100644
> --- a/Documentation/networking/filter.rst
> +++ b/Documentation/networking/filter.rst
> @@ -1006,13 +1006,13 @@ Size modifier is one of ...
>   
>   Mode modifier is one of::
>   
> -  BPF_IMM  0x00  /* used for 32-bit mov in classic BPF and 64-bit in eBPF */
> -  BPF_ABS  0x20
> -  BPF_IND  0x40
> -  BPF_MEM  0x60
> -  BPF_LEN  0x80  /* classic BPF only, reserved in eBPF */
> -  BPF_MSH  0xa0  /* classic BPF only, reserved in eBPF */
> -  BPF_XADD 0xc0  /* eBPF only, exclusive add */
> +  BPF_IMM     0x00  /* used for 32-bit mov in classic BPF and 64-bit in eBPF */
> +  BPF_ABS     0x20
> +  BPF_IND     0x40
> +  BPF_MEM     0x60
> +  BPF_LEN     0x80  /* classic BPF only, reserved in eBPF */
> +  BPF_MSH     0xa0  /* classic BPF only, reserved in eBPF */
> +  BPF_ATOMIC  0xc0  /* eBPF only, atomic operations */
>   
>   eBPF has two non-generic instructions: (BPF_ABS | <size> | BPF_LD) and
>   (BPF_IND | <size> | BPF_LD) which are used to access packet data.
> @@ -1044,11 +1044,16 @@ Unlike classic BPF instruction set, eBPF has generic load/store operations::
>       BPF_MEM | <size> | BPF_STX:  *(size *) (dst_reg + off) = src_reg
>       BPF_MEM | <size> | BPF_ST:   *(size *) (dst_reg + off) = imm32
>       BPF_MEM | <size> | BPF_LDX:  dst_reg = *(size *) (src_reg + off)
> -    BPF_XADD | BPF_W  | BPF_STX: lock xadd *(u32 *)(dst_reg + off16) += src_reg
> -    BPF_XADD | BPF_DW | BPF_STX: lock xadd *(u64 *)(dst_reg + off16) += src_reg
>   
> -Where size is one of: BPF_B or BPF_H or BPF_W or BPF_DW. Note that 1 and
> -2 byte atomic increments are not supported.
> +Where size is one of: BPF_B or BPF_H or BPF_W or BPF_DW.
> +
> +It also includes atomic operations, which use the immediate field for extra
> +encoding.
> +
> +   BPF_ADD, BPF_ATOMIC | BPF_W  | BPF_STX: lock xadd *(u32 *)(dst_reg + off16) += src_reg
> +   BPF_ADD, BPF_ATOMIC | BPF_DW | BPF_STX: lock xadd *(u64 *)(dst_reg + off16) += src_reg
> +
> +Note that 1 and 2 byte atomic operations are not supported.
>   
>   eBPF has one 16-byte instruction: BPF_LD | BPF_DW | BPF_IMM which consists
>   of two consecutive ``struct bpf_insn`` 8-byte blocks and interpreted as single
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index 0207b6ea6e8a..897634d0a67c 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -1620,10 +1620,9 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
>   		}
>   		emit_str_r(dst_lo, tmp2, off, ctx, BPF_SIZE(code));
>   		break;
> -	/* STX XADD: lock *(u32 *)(dst + off) += src */
> -	case BPF_STX | BPF_XADD | BPF_W:
> -	/* STX XADD: lock *(u64 *)(dst + off) += src */
> -	case BPF_STX | BPF_XADD | BPF_DW:
> +	/* Atomic ops */
> +	case BPF_STX | BPF_ATOMIC | BPF_W:
> +	case BPF_STX | BPF_ATOMIC | BPF_DW:
>   		goto notyet;
>   	/* STX: *(size *)(dst + off) = src */
>   	case BPF_STX | BPF_MEM | BPF_W:
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index ef9f1d5e989d..f7b194878a99 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -875,10 +875,18 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>   		}
>   		break;
>   
> -	/* STX XADD: lock *(u32 *)(dst + off) += src */
> -	case BPF_STX | BPF_XADD | BPF_W:
> -	/* STX XADD: lock *(u64 *)(dst + off) += src */
> -	case BPF_STX | BPF_XADD | BPF_DW:
> +	case BPF_STX | BPF_ATOMIC | BPF_W:
> +	case BPF_STX | BPF_ATOMIC | BPF_DW:
> +		if (insn->imm != BPF_ADD) {

Currently BPF_ADD (although it is 0) is encoded at bit 4-7 of imm.
Do you think we should encode it in 0-3 to make such a comparision
and subsequent insn->imm = BPF_ADD making more sense?


> +			pr_err_once("unknown atomic op code %02x\n", insn->imm);
> +			return -EINVAL;
> +		}
> +
> +		/* STX XADD: lock *(u32 *)(dst + off) += src
> +		 * and
> +		 * STX XADD: lock *(u64 *)(dst + off) += src
> +		 */
> +
>   		if (!off) {
>   			reg = dst;
>   		} else {
[...]
> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> index 0a721f6e8676..0767d7b579e9 100644
> --- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> +++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> @@ -3109,13 +3109,19 @@ mem_xadd(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta, bool is64)
>   	return 0;
>   }
>   
> -static int mem_xadd4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> +static int mem_atm4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>   {
> +	if (meta->insn.off != BPF_ADD)
> +		return -EOPNOTSUPP;

meta->insn.imm?

> +
>   	return mem_xadd(nfp_prog, meta, false);
>   }
>   
> -static int mem_xadd8(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> +static int mem_atm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>   {
> +	if (meta->insn.off != BPF_ADD)

meta->insn.imm?

> +		return -EOPNOTSUPP;
> +
>   	return mem_xadd(nfp_prog, meta, true);
>   }
>   
> @@ -3475,8 +3481,8 @@ static const instr_cb_t instr_cb[256] = {
>   	[BPF_STX | BPF_MEM | BPF_H] =	mem_stx2,
>   	[BPF_STX | BPF_MEM | BPF_W] =	mem_stx4,
>   	[BPF_STX | BPF_MEM | BPF_DW] =	mem_stx8,
> -	[BPF_STX | BPF_XADD | BPF_W] =	mem_xadd4,
> -	[BPF_STX | BPF_XADD | BPF_DW] =	mem_xadd8,
> +	[BPF_STX | BPF_ATOMIC | BPF_W] =	mem_atm4,
> +	[BPF_STX | BPF_ATOMIC | BPF_DW] =	mem_atm8,
>   	[BPF_ST | BPF_MEM | BPF_B] =	mem_st1,
>   	[BPF_ST | BPF_MEM | BPF_H] =	mem_st2,
>   	[BPF_ST | BPF_MEM | BPF_W] =	mem_st4,
[...]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 7/7] bpf: Add tests for new BPF atomic operations
  2020-11-23 17:32 ` [PATCH 7/7] bpf: Add tests for new BPF atomic operations Brendan Jackman
@ 2020-11-24  0:26   ` Yonghong Song
  2020-11-24 13:10     ` Brendan Jackman
  0 siblings, 1 reply; 29+ messages in thread
From: Yonghong Song @ 2020-11-24  0:26 UTC (permalink / raw)
  To: Brendan Jackman, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, KP Singh, Florent Revest



On 11/23/20 9:32 AM, Brendan Jackman wrote:
> This relies on the work done by Yonghong Song in
> https://reviews.llvm.org/D72184
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>   tools/testing/selftests/bpf/Makefile          |   2 +-
>   .../selftests/bpf/prog_tests/atomics_test.c   | 145 ++++++++++++++++++
>   .../selftests/bpf/progs/atomics_test.c        |  61 ++++++++
>   .../selftests/bpf/verifier/atomic_cmpxchg.c   |  96 ++++++++++++
>   .../selftests/bpf/verifier/atomic_fetch_add.c | 106 +++++++++++++
>   .../selftests/bpf/verifier/atomic_xchg.c      | 113 ++++++++++++++
>   6 files changed, 522 insertions(+), 1 deletion(-)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/atomics_test.c
>   create mode 100644 tools/testing/selftests/bpf/progs/atomics_test.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_fetch_add.c
>   create mode 100644 tools/testing/selftests/bpf/verifier/atomic_xchg.c
> 
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 3d5940cd110d..4e28640ca2d8 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -250,7 +250,7 @@ define CLANG_BPF_BUILD_RULE
>   	$(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2)
>   	$(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm			\
>   		-c $1 -o - || echo "BPF obj compilation failed") | 	\
> -	$(LLC) -mattr=dwarfris -march=bpf -mcpu=v3 $4 -filetype=obj -o $2
> +	$(LLC) -mattr=dwarfris -march=bpf -mcpu=v4 $4 -filetype=obj -o $2

We have an issue here. If we change -mcpu=v4 here, we will force
people to use trunk llvm to run selftests which is not a good idea.

I am wondering whether we can single out progs/atomics_test.c, which 
will be compiled with -mcpu=v4 and run with test_progs.

test_progs-no_alu32 runs tests without alu32. Since -mcpu=v4 implies
alu32, atomic tests should be skipped in test_progs-no-alu32.

In bpf_helpers.h, we already use __clang_major__ macro to compare
to clang version,

#if __clang_major__ >= 8 && defined(__bpf__)
static __always_inline void
bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
{
         if (!__builtin_constant_p(slot))
                 __bpf_unreachable();
...

I think we could also use __clang_major__ in progs/atomics_test.c
to enable tested intrinsics only if __clang_major__ >= 12? This
way, the same code can be compiled with -mcpu=v2/v3.

Alternatively, you can also use a macro at clang command line like
    clang -mcpu=v4 -DENABLE_ATOMIC ...
    clang -mcpu=v3/v2 ...

progs/atomics_test.c:
    #ifdef ENABLE_ATOMIC
      ... atomic_intrinsics ...
    #endif


>   endef
>   # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
>   define CLANG_NOALU32_BPF_BUILD_RULE
> diff --git a/tools/testing/selftests/bpf/prog_tests/atomics_test.c b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
> new file mode 100644
> index 000000000000..a4859d88fc11
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +
> +#include "atomics_test.skel.h"
> +
> +static void test_add(void)
> +{
> +	struct atomics_test *atomics_skel = NULL;
> +	int err, prog_fd;
> +	__u32 duration = 0, retval;
> +
> +	atomics_skel = atomics_test__open_and_load();
> +	if (CHECK(!atomics_skel, "atomics_skel_load", "atomics skeleton failed\n"))
> +		goto cleanup;
> +
> +	err = atomics_test__attach(atomics_skel);
> +	if (CHECK(err, "atomics_attach", "atomics attach failed: %d\n", err))
> +		goto cleanup;
> +
> +	prog_fd = bpf_program__fd(atomics_skel->progs.add);
> +	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> +				NULL, NULL, &retval, &duration);
> +	if (CHECK(err || retval, "test_run add",
> +		  "err %d errno %d retval %d duration %d\n",
> +		  err, errno, retval, duration))
> +		goto cleanup;
> +
> +	CHECK(atomics_skel->data->add64_value != 3, "add64_value",
> +	      "64bit atomic add value was not incremented (got %lld want 2)\n",
> +	      atomics_skel->data->add64_value);
> +	CHECK(atomics_skel->bss->add64_result != 1, "add64_result",
> +	      "64bit atomic add bad return value (got %lld want 1)\n",
> +	      atomics_skel->bss->add64_result);
> +
> +	CHECK(atomics_skel->data->add32_value != 3, "add32_value",
> +	      "32bit atomic add value was not incremented (got %d want 2)\n",
> +	      atomics_skel->data->add32_value);
> +	CHECK(atomics_skel->bss->add32_result != 1, "add32_result",
> +	      "32bit atomic add bad return value (got %d want 1)\n",
> +	      atomics_skel->bss->add32_result);
> +
> +	CHECK(atomics_skel->bss->add_stack_value_copy != 3, "add_stack_value",
> +	      "_stackbit atomic add value was not incremented (got %lld want 2)\n",
> +	      atomics_skel->bss->add_stack_value_copy);
> +	CHECK(atomics_skel->bss->add_stack_result != 1, "add_stack_result",
> +	      "_stackbit atomic add bad return value (got %lld want 1)\n",
> +	      atomics_skel->bss->add_stack_result);
> +
> +cleanup:
> +	atomics_test__destroy(atomics_skel);
> +}
> +
[...]
> diff --git a/tools/testing/selftests/bpf/progs/atomics_test.c b/tools/testing/selftests/bpf/progs/atomics_test.c
> new file mode 100644
> index 000000000000..d81f45eb6c45
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/atomics_test.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +__u64 add64_value = 1;
> +__u64 add64_result;
> +__u32 add32_value = 1;
> +__u32 add32_result;
> +__u64 add_stack_value_copy;
> +__u64 add_stack_result;

To please llvm10, let us initialize all unitialized globals explicitly like
    __u64 add64_result = 0;
    __u32 add32_result = 0;
    ...

llvm11 and above are okay but llvm10 put those uninitialized globals
into COM section (not .bss or .data sections) which BTF did not
handle them.

> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(add, int a)
> +{
> +	__u64 add_stack_value = 1;
> +
> +	add64_result = __sync_fetch_and_add(&add64_value, 2);
> +	add32_result = __sync_fetch_and_add(&add32_value, 2);
> +	add_stack_result = __sync_fetch_and_add(&add_stack_value, 2);
> +	add_stack_value_copy = add_stack_value;
> +
> +	return 0;
> +}
> +
> +__u64 cmpxchg64_value = 1;
> +__u64 cmpxchg64_result_fail;
> +__u64 cmpxchg64_result_succeed;
> +__u32 cmpxchg32_value = 1;
> +__u32 cmpxchg32_result_fail;
> +__u32 cmpxchg32_result_succeed;

same here. explicitly initializing cmpxchg64_result_fail, 
cmpxchg64_result_succeed, cmpxchg32_result_fail, cmpxchg32_result_succeed.

> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(cmpxchg, int a)
> +{
> +	cmpxchg64_result_fail = __sync_val_compare_and_swap(
> +		&cmpxchg64_value, 0, 3);
> +	cmpxchg64_result_succeed = __sync_val_compare_and_swap(
> +		&cmpxchg64_value, 1, 2);
> +
> +	cmpxchg32_result_fail = __sync_val_compare_and_swap(
> +		&cmpxchg32_value, 0, 3);
> +	cmpxchg32_result_succeed = __sync_val_compare_and_swap(
> +		&cmpxchg32_value, 1, 2);
> +
> +	return 0;
> +}
> +
> +__u64 xchg64_value = 1;
> +__u64 xchg64_result;
> +__u32 xchg32_value = 1;
> +__u32 xchg32_result;

explicitly initializing xchg64_result, xchg32_result.

> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(xchg, int a)
> +{
> +	__u64 val64 = 2;
> +	__u32 val32 = 2;
> +
> +	__atomic_exchange(&xchg64_value, &val64, &xchg64_result, __ATOMIC_RELAXED);
> +	__atomic_exchange(&xchg32_value, &val32, &xchg32_result, __ATOMIC_RELAXED);
> +
> +	return 0;
> +}
[...]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  2020-11-23 17:31 ` [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
@ 2020-11-24  3:28     ` kernel test robot
  2020-11-24  3:28     ` kernel test robot
  2020-11-24  6:50   ` Alexei Starovoitov
  2 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-11-24  3:28 UTC (permalink / raw)
  To: Brendan Jackman, bpf
  Cc: kbuild-all, clang-built-linux, Alexei Starovoitov, Yonghong Song,
	Daniel Borkmann, KP Singh, Florent Revest, Brendan Jackman

[-- Attachment #1: Type: text/plain, Size: 8949 bytes --]

Hi Brendan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on bpf/master powerpc/next linus/master v5.10-rc5 next-20201123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brendan-Jackman/Atomics-for-eBPF/20201124-013549
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: powerpc-randconfig-r001-20201123 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project df9ae5992889560a8f3c6760b54d5051b47c7bf5)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/d8b7356263922e2ef6596247034c6b5273d2a8b9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brendan-Jackman/Atomics-for-eBPF/20201124-013549
        git checkout d8b7356263922e2ef6596247034c6b5273d2a8b9
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/ethernet/netronome/nfp/bpf/verifier.c:482:7: error: implicit declaration of function 'is_mbpf_xadd' [-Werror,-Wimplicit-function-declaration]
                   if (is_mbpf_xadd(meta)) {
                       ^
   1 error generated.
--
>> drivers/net/ethernet/netronome/nfp/bpf/jit.c:3485:36: error: use of undeclared identifier 'mem_atm8'
           [BPF_STX | BPF_ATOMIC | BPF_DW] =       mem_atm8,
                                                   ^
   1 error generated.

vim +/is_mbpf_xadd +482 drivers/net/ethernet/netronome/nfp/bpf/verifier.c

dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  449  
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  450  static int
2ca71441f524b0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-12  451  nfp_bpf_check_ptr(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
638f5b90d46016 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Alexei Starovoitov 2017-10-31  452  		  struct bpf_verifier_env *env, u8 reg_no)
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  453  {
638f5b90d46016 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Alexei Starovoitov 2017-10-31  454  	const struct bpf_reg_state *reg = cur_regs(env) + reg_no;
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  455  	int err;
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  456  
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  457  	if (reg->type != PTR_TO_CTX &&
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  458  	    reg->type != PTR_TO_STACK &&
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  459  	    reg->type != PTR_TO_MAP_VALUE &&
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  460  	    reg->type != PTR_TO_PACKET) {
ff627e3d07a07f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Quentin Monnet     2018-01-10  461  		pr_vlog(env, "unsupported ptr type: %d\n", reg->type);
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  462  		return -EINVAL;
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  463  	}
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  464  
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  465  	if (reg->type == PTR_TO_STACK) {
ff627e3d07a07f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Quentin Monnet     2018-01-10  466  		err = nfp_bpf_check_stack_access(nfp_prog, meta, reg, env);
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  467  		if (err)
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  468  			return err;
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  469  	}
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  470  
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  471  	if (reg->type == PTR_TO_MAP_VALUE) {
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  472  		if (is_mbpf_load(meta)) {
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  473  			err = nfp_bpf_map_mark_used(env, meta, reg,
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  474  						    NFP_MAP_USE_READ);
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  475  			if (err)
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  476  				return err;
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  477  		}
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  478  		if (is_mbpf_store(meta)) {
7dfa4d87cfc48f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-16  479  			pr_vlog(env, "map writes not supported\n");
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  480  			return -EOPNOTSUPP;
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  481  		}
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28 @482  		if (is_mbpf_xadd(meta)) {
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  483  			err = nfp_bpf_map_mark_used(env, meta, reg,
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  484  						    NFP_MAP_USE_ATOMIC_CNT);
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  485  			if (err)
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  486  				return err;
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  487  		}
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  488  	}
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  489  
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  490  	if (meta->ptr.type != NOT_INIT && meta->ptr.type != reg->type) {
ff627e3d07a07f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Quentin Monnet     2018-01-10  491  		pr_vlog(env, "ptr type changed for instruction %d -> %d\n",
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  492  			meta->ptr.type, reg->type);
2ca71441f524b0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-12  493  		return -EINVAL;
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  494  	}
2ca71441f524b0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-12  495  
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  496  	meta->ptr = *reg;
2ca71441f524b0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-12  497  
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  498  	return 0;
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  499  }
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  500  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39400 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
@ 2020-11-24  3:28     ` kernel test robot
  0 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-11-24  3:28 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 9050 bytes --]

Hi Brendan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on bpf/master powerpc/next linus/master v5.10-rc5 next-20201123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brendan-Jackman/Atomics-for-eBPF/20201124-013549
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: powerpc-randconfig-r001-20201123 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project df9ae5992889560a8f3c6760b54d5051b47c7bf5)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/d8b7356263922e2ef6596247034c6b5273d2a8b9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brendan-Jackman/Atomics-for-eBPF/20201124-013549
        git checkout d8b7356263922e2ef6596247034c6b5273d2a8b9
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/ethernet/netronome/nfp/bpf/verifier.c:482:7: error: implicit declaration of function 'is_mbpf_xadd' [-Werror,-Wimplicit-function-declaration]
                   if (is_mbpf_xadd(meta)) {
                       ^
   1 error generated.
--
>> drivers/net/ethernet/netronome/nfp/bpf/jit.c:3485:36: error: use of undeclared identifier 'mem_atm8'
           [BPF_STX | BPF_ATOMIC | BPF_DW] =       mem_atm8,
                                                   ^
   1 error generated.

vim +/is_mbpf_xadd +482 drivers/net/ethernet/netronome/nfp/bpf/verifier.c

dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  449  
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  450  static int
2ca71441f524b0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-12  451  nfp_bpf_check_ptr(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
638f5b90d46016 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Alexei Starovoitov 2017-10-31  452  		  struct bpf_verifier_env *env, u8 reg_no)
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  453  {
638f5b90d46016 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Alexei Starovoitov 2017-10-31  454  	const struct bpf_reg_state *reg = cur_regs(env) + reg_no;
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  455  	int err;
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  456  
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  457  	if (reg->type != PTR_TO_CTX &&
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  458  	    reg->type != PTR_TO_STACK &&
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  459  	    reg->type != PTR_TO_MAP_VALUE &&
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  460  	    reg->type != PTR_TO_PACKET) {
ff627e3d07a07f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Quentin Monnet     2018-01-10  461  		pr_vlog(env, "unsupported ptr type: %d\n", reg->type);
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  462  		return -EINVAL;
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  463  	}
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  464  
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  465  	if (reg->type == PTR_TO_STACK) {
ff627e3d07a07f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Quentin Monnet     2018-01-10  466  		err = nfp_bpf_check_stack_access(nfp_prog, meta, reg, env);
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  467  		if (err)
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  468  			return err;
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  469  	}
ee9133a845fe8a drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  470  
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  471  	if (reg->type == PTR_TO_MAP_VALUE) {
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  472  		if (is_mbpf_load(meta)) {
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  473  			err = nfp_bpf_map_mark_used(env, meta, reg,
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  474  						    NFP_MAP_USE_READ);
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  475  			if (err)
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  476  				return err;
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  477  		}
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  478  		if (is_mbpf_store(meta)) {
7dfa4d87cfc48f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-16  479  			pr_vlog(env, "map writes not supported\n");
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  480  			return -EOPNOTSUPP;
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  481  		}
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28 @482  		if (is_mbpf_xadd(meta)) {
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  483  			err = nfp_bpf_map_mark_used(env, meta, reg,
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  484  						    NFP_MAP_USE_ATOMIC_CNT);
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  485  			if (err)
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  486  				return err;
dcb0c27f3c989f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-03-28  487  		}
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  488  	}
3dd43c3319cb0b drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2018-01-11  489  
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  490  	if (meta->ptr.type != NOT_INIT && meta->ptr.type != reg->type) {
ff627e3d07a07f drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Quentin Monnet     2018-01-10  491  		pr_vlog(env, "ptr type changed for instruction %d -> %d\n",
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  492  			meta->ptr.type, reg->type);
2ca71441f524b0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-12  493  		return -EINVAL;
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  494  	}
2ca71441f524b0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-12  495  
70c78fc138b6d0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-23  496  	meta->ptr = *reg;
2ca71441f524b0 drivers/net/ethernet/netronome/nfp/bpf/verifier.c     Jakub Kicinski     2017-10-12  497  
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  498  	return 0;
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  499  }
cd7df56ed3e60d drivers/net/ethernet/netronome/nfp/nfp_bpf_verifier.c Jakub Kicinski     2016-09-21  500  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39400 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends
  2020-11-23 17:32 ` [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends Brendan Jackman
  2020-11-23 19:29   ` Brendan Jackman
@ 2020-11-24  6:40   ` Alexei Starovoitov
  2020-11-24 10:55     ` Brendan Jackman
  1 sibling, 1 reply; 29+ messages in thread
From: Alexei Starovoitov @ 2020-11-24  6:40 UTC (permalink / raw)
  To: Brendan Jackman
  Cc: bpf, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest

On Mon, Nov 23, 2020 at 05:32:01PM +0000, Brendan Jackman wrote:
> These are the operations that implement atomic exchange and
> compare-exchange.
> 
> They are peculiarly named because of the presence of the separate
> FETCH field that tells you whether the instruction writes the value
> back to the src register. Neither operation is supported without
> BPF_FETCH:
> 
> - BPF_CMPSET without BPF_FETCH (i.e. an atomic compare-and-set
>   without knowing whether the write was successfully) isn't implemented
>   by the kernel, x86, or ARM. It would be a burden on the JIT and it's
>   hard to imagine a use for this operation, so it's not supported.
> 
> - BPF_SET without BPF_FETCH would be bpf_set, which has pretty
>   limited use: all it really lets you do is atomically set 64-bit
>   values on 32-bit CPUs. It doesn't imply any barriers.

...

> -			if (insn->imm & BPF_FETCH) {
> +			switch (insn->imm) {
> +			case BPF_SET | BPF_FETCH:
> +				/* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
> +				EMIT1(0x87);
> +				break;
> +			case BPF_CMPSET | BPF_FETCH:
> +				/* r0 = atomic_cmpxchg(*(u32/u64*)(dst_reg + off), r0, src_reg); */
> +				EMIT2(0x0F, 0xB1);
> +				break;
...
>  /* atomic op type fields (stored in immediate) */
> +#define BPF_SET		0xe0	/* atomic write */
> +#define BPF_CMPSET	0xf0	/* atomic compare-and-write */
> +
>  #define BPF_FETCH	0x01	/* fetch previous value into src reg */

I think SET in the name looks odd.
I understand that you picked this name so that SET|FETCH together would form
more meaningful combination of words, but we're not planning to support SET
alone. There is no such instruction in a cpu. If we ever do test_and_set it
would be something different.
How about the following instead:
+#define BPF_XCHG	0xe1	/* atomic exchange */
+#define BPF_CMPXCHG	0xf1	/* atomic compare exchange */
In other words get that fetch bit right away into the encoding.
Then the switch statement above could be:
+			switch (insn->imm) {
+			case BPF_XCHG:
+				/* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
+				EMIT1(0x87);
...
+			case BPF_ADD | BPF_FETCH:
...
+			case BPF_ADD:

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  2020-11-23 17:31 ` [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
  2020-11-23 23:54   ` Yonghong Song
  2020-11-24  3:28     ` kernel test robot
@ 2020-11-24  6:50   ` Alexei Starovoitov
  2020-11-24 11:21     ` Brendan Jackman
  2 siblings, 1 reply; 29+ messages in thread
From: Alexei Starovoitov @ 2020-11-24  6:50 UTC (permalink / raw)
  To: Brendan Jackman
  Cc: bpf, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest

On Mon, Nov 23, 2020 at 05:31:58PM +0000, 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           | 27 +++++++++-------
>  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                  | 26 +++++++++-------
>  arch/sparc/net/bpf_jit_comp_64.c              | 14 +++++++--
>  arch/x86/net/bpf_jit_comp.c                   | 30 +++++++++++-------
>  arch/x86/net/bpf_jit_comp32.c                 |  6 ++--

I think this massive rename is not needed.
BPF_XADD is uapi and won't be removed.
Updating documentation, samples and tests is probably enough.

> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 1b62397bd124..ce19988fb312 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -261,13 +261,15 @@ static inline bool insn_is_zext(const struct bpf_insn *insn)
>  
>  /* Atomic memory add, *(uint *)(dst_reg + off16) += src_reg */
>  
> -#define BPF_STX_XADD(SIZE, DST, SRC, OFF)			\
> +#define BPF_ATOMIC_ADD(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 })
> +#define BPF_STX_XADD BPF_ATOMIC_ADD /* alias */

this is fine.

> +
>  
>  /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
>  
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3ca6146f001a..dcd08783647d 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -19,7 +19,8 @@
>  
>  /* ld/ldx fields */
>  #define BPF_DW		0x18	/* double word (64-bit) */
> -#define BPF_XADD	0xc0	/* exclusive add */
> +#define BPF_ATOMIC	0xc0	/* atomic memory ops - op type in immediate */
> +#define BPF_XADD	0xc0	/* legacy name, don't add new uses */

I think the comment sounds too strong.
New uses of BPF_XADD are discouraged, but they're still correct.

>  
>  /* alu/jmp fields */
>  #define BPF_MOV		0xb0	/* mov reg to reg */
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index ff55cbcfbab4..48b192a8edce 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1317,8 +1317,8 @@ EXPORT_SYMBOL_GPL(__bpf_call_base);
>  	INSN_3(STX, MEM,  H),			\
>  	INSN_3(STX, MEM,  W),			\
>  	INSN_3(STX, MEM,  DW),			\
> -	INSN_3(STX, XADD, W),			\
> -	INSN_3(STX, XADD, DW),			\
> +	INSN_3(STX, ATOMIC, W),			\
> +	INSN_3(STX, ATOMIC, DW),		\
>  	/*   Immediate based. */		\
>  	INSN_3(ST, MEM, B),			\
>  	INSN_3(ST, MEM, H),			\
> @@ -1626,13 +1626,25 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
>  	LDX_PROBE(DW, 8)
>  #undef LDX_PROBE
>  
> -	STX_XADD_W: /* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
> -		atomic_add((u32) SRC, (atomic_t *)(unsigned long)
> -			   (DST + insn->off));
> +	STX_ATOMIC_W:
> +		switch (insn->imm) {
> +		case BPF_ADD:
> +			/* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
> +			atomic_add((u32) SRC, (atomic_t *)(unsigned long)
> +				   (DST + insn->off));
> +		default:
> +			goto default_label;
> +		}
>  		CONT;
> -	STX_XADD_DW: /* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
> -		atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
> -			     (DST + insn->off));
> +	STX_ATOMIC_DW:
> +		switch (insn->imm) {
> +		case BPF_ADD:
> +			/* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
> +			atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
> +				     (DST + insn->off));
> +		default:
> +			goto default_label;
> +		}

+1

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fb2943ea715d..06885e2501f8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3598,13 +3598,17 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>  	return err;
>  }
>  
> -static int check_xadd(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
> +static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)

+1

> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index ca7d635bccd9..fbb13ef9207c 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -4295,7 +4295,7 @@ static struct bpf_test tests[] = {
>  		{ { 0, 0xffffffff } },
>  		.stack_depth = 40,
>  	},
> -	/* BPF_STX | BPF_XADD | BPF_W/DW */
> +	/* BPF_STX | BPF_ATOMIC | BPF_W/DW */

I would leave this comment as-is.
There are several uses of BPF_STX_XADD in that file.
So the comment is fine.

>  	{
>  		"STX_XADD_W: Test: 0x12 + 0x10 = 0x22",
>  		.u.insns_int = {
> diff --git a/samples/bpf/bpf_insn.h b/samples/bpf/bpf_insn.h
> index 544237980582..db67a2847395 100644
> --- a/samples/bpf/bpf_insn.h
> +++ b/samples/bpf/bpf_insn.h
> @@ -138,11 +138,11 @@ struct bpf_insn;
>  
>  #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 */
>  
> diff --git a/samples/bpf/sock_example.c b/samples/bpf/sock_example.c
> index 00aae1d33fca..41ec3ca3bc40 100644
> --- a/samples/bpf/sock_example.c
> +++ b/samples/bpf/sock_example.c
> @@ -54,7 +54,8 @@ static int test_sock(void)
>  		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
>  		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
>  		BPF_MOV64_IMM(BPF_REG_1, 1), /* r1 = 1 */
> -		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
> +		BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
> +			     BPF_REG_0, BPF_REG_1, 0, BPF_ADD), /* xadd r0 += r1 */

+1

> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
> index b549fcfacc0b..79401a59a988 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
> @@ -45,13 +45,15 @@ static int prog_load_cnt(int verdict, int val)
>  		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
>  		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
>  		BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = 1 */
> -		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
> +		BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
> +			     BPF_REG_0, BPF_REG_1, 0, BPF_ADD), /* xadd r0 += r1 */

+1

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
  2020-11-23 17:32 ` [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
  2020-11-23 21:12     ` kernel test robot
  2020-11-23 21:51     ` kernel test robot
@ 2020-11-24  6:52   ` Alexei Starovoitov
  2020-11-24 10:48     ` Brendan Jackman
  2 siblings, 1 reply; 29+ messages in thread
From: Alexei Starovoitov @ 2020-11-24  6:52 UTC (permalink / raw)
  To: Brendan Jackman
  Cc: bpf, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest

On Mon, Nov 23, 2020 at 05:32:00PM +0000, Brendan Jackman wrote:
> @@ -3644,8 +3649,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>  		return err;
>  
>  	/* check whether we can write into the same memory */
> -	return check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> -				BPF_SIZE(insn->code), BPF_WRITE, -1, true);
> +	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> +			       BPF_SIZE(insn->code), BPF_WRITE, -1, true);
> +	if (err)
> +		return err;
> +
> +	if (!(insn->imm & BPF_FETCH))
> +		return 0;
> +
> +	/* check and record load of old value into src reg  */
> +	err = check_reg_arg(env, insn->src_reg, DST_OP);
> +	if (err)
> +		return err;
> +	regs[insn->src_reg].type = SCALAR_VALUE;

check_reg_arg() will call mark_reg_unknown() which will set type to SCALAR_VALUE.
What is the point of another assignment?

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
  2020-11-24  6:52   ` Alexei Starovoitov
@ 2020-11-24 10:48     ` Brendan Jackman
  0 siblings, 0 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-24 10:48 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest

On Mon, Nov 23, 2020 at 10:52:57PM -0800, Alexei Starovoitov wrote:
> On Mon, Nov 23, 2020 at 05:32:00PM +0000, Brendan Jackman wrote:
> > @@ -3644,8 +3649,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
> >  		return err;
> >  
> >  	/* check whether we can write into the same memory */
> > -	return check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> > -				BPF_SIZE(insn->code), BPF_WRITE, -1, true);
> > +	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> > +			       BPF_SIZE(insn->code), BPF_WRITE, -1, true);
> > +	if (err)
> > +		return err;
> > +
> > +	if (!(insn->imm & BPF_FETCH))
> > +		return 0;
> > +
> > +	/* check and record load of old value into src reg  */
> > +	err = check_reg_arg(env, insn->src_reg, DST_OP);
> > +	if (err)
> > +		return err;
> > +	regs[insn->src_reg].type = SCALAR_VALUE;
> 
> check_reg_arg() will call mark_reg_unknown() which will set type to SCALAR_VALUE.
> What is the point of another assignment?

Yep, this is just an oversight - thanks, will remove.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends
  2020-11-24  6:40   ` Alexei Starovoitov
@ 2020-11-24 10:55     ` Brendan Jackman
  2020-11-24 22:51       ` Alexei Starovoitov
  0 siblings, 1 reply; 29+ messages in thread
From: Brendan Jackman @ 2020-11-24 10:55 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest

On Mon, Nov 23, 2020 at 10:40:00PM -0800, Alexei Starovoitov wrote:
> On Mon, Nov 23, 2020 at 05:32:01PM +0000, Brendan Jackman wrote:
> > These are the operations that implement atomic exchange and
> > compare-exchange.
> > 
> > They are peculiarly named because of the presence of the separate
> > FETCH field that tells you whether the instruction writes the value
> > back to the src register. Neither operation is supported without
> > BPF_FETCH:
> > 
> > - BPF_CMPSET without BPF_FETCH (i.e. an atomic compare-and-set
> >   without knowing whether the write was successfully) isn't implemented
> >   by the kernel, x86, or ARM. It would be a burden on the JIT and it's
> >   hard to imagine a use for this operation, so it's not supported.
> > 
> > - BPF_SET without BPF_FETCH would be bpf_set, which has pretty
> >   limited use: all it really lets you do is atomically set 64-bit
> >   values on 32-bit CPUs. It doesn't imply any barriers.
> 
> ...
> 
> > -			if (insn->imm & BPF_FETCH) {
> > +			switch (insn->imm) {
> > +			case BPF_SET | BPF_FETCH:
> > +				/* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
> > +				EMIT1(0x87);
> > +				break;
> > +			case BPF_CMPSET | BPF_FETCH:
> > +				/* r0 = atomic_cmpxchg(*(u32/u64*)(dst_reg + off), r0, src_reg); */
> > +				EMIT2(0x0F, 0xB1);
> > +				break;
> ...
> >  /* atomic op type fields (stored in immediate) */
> > +#define BPF_SET		0xe0	/* atomic write */
> > +#define BPF_CMPSET	0xf0	/* atomic compare-and-write */
> > +
> >  #define BPF_FETCH	0x01	/* fetch previous value into src reg */
> 
> I think SET in the name looks odd.
> I understand that you picked this name so that SET|FETCH together would form
> more meaningful combination of words, but we're not planning to support SET
> alone. There is no such instruction in a cpu. If we ever do test_and_set it
> would be something different.

Yeah this makes sense...

> How about the following instead:
> +#define BPF_XCHG	0xe1	/* atomic exchange */
> +#define BPF_CMPXCHG	0xf1	/* atomic compare exchange */
> In other words get that fetch bit right away into the encoding.
> Then the switch statement above could be:
> +			switch (insn->imm) {
> +			case BPF_XCHG:
> +				/* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
> +				EMIT1(0x87);
> ...
> +			case BPF_ADD | BPF_FETCH:
> ...
> +			case BPF_ADD:

... Although I'm a little wary of this because it makes it very messy to
do something like switch(BPF_OP(insn->imm)) since we'd have no name for
BPF_OP(0xe1). That might be fine - I haven't needed such a construction
so far (although I have used BPF_OP(insn->imm)) so maybe we wouldn't
ever need it.

What do you think? Maybe we add the `#define BPF_XCHG 0xe1` and then if we
later need to do switch(BPF_OP(insn->imm)) we could bring back
`#define BPF_SET 0xe` as needed?

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  2020-11-23 23:54   ` Yonghong Song
@ 2020-11-24 11:02     ` Brendan Jackman
  2020-11-24 16:04       ` Yonghong Song
  0 siblings, 1 reply; 29+ messages in thread
From: Brendan Jackman @ 2020-11-24 11:02 UTC (permalink / raw)
  To: Yonghong Song
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, KP Singh, Florent Revest

On Mon, Nov 23, 2020 at 03:54:38PM -0800, Yonghong Song wrote:
> 
> 
> On 11/23/20 9:31 AM, Brendan Jackman wrote:
...
> > diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> > index 0207b6ea6e8a..897634d0a67c 100644
> > --- a/arch/arm/net/bpf_jit_32.c
> > +++ b/arch/arm/net/bpf_jit_32.c
> > @@ -1620,10 +1620,9 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
> >   		}
> >   		emit_str_r(dst_lo, tmp2, off, ctx, BPF_SIZE(code));
> >   		break;
> > -	/* STX XADD: lock *(u32 *)(dst + off) += src */
> > -	case BPF_STX | BPF_XADD | BPF_W:
> > -	/* STX XADD: lock *(u64 *)(dst + off) += src */
> > -	case BPF_STX | BPF_XADD | BPF_DW:
> > +	/* Atomic ops */
> > +	case BPF_STX | BPF_ATOMIC | BPF_W:
> > +	case BPF_STX | BPF_ATOMIC | BPF_DW:
> >   		goto notyet;
> >   	/* STX: *(size *)(dst + off) = src */
> >   	case BPF_STX | BPF_MEM | BPF_W:
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index ef9f1d5e989d..f7b194878a99 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -875,10 +875,18 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
> >   		}
> >   		break;
> > -	/* STX XADD: lock *(u32 *)(dst + off) += src */
> > -	case BPF_STX | BPF_XADD | BPF_W:
> > -	/* STX XADD: lock *(u64 *)(dst + off) += src */
> > -	case BPF_STX | BPF_XADD | BPF_DW:
> > +	case BPF_STX | BPF_ATOMIC | BPF_W:
> > +	case BPF_STX | BPF_ATOMIC | BPF_DW:
> > +		if (insn->imm != BPF_ADD) {
> 
> Currently BPF_ADD (although it is 0) is encoded at bit 4-7 of imm.
> Do you think we should encode it in 0-3 to make such a comparision
> and subsequent insn->imm = BPF_ADD making more sense?

Sorry not quite sure what you mean by this... I think encoding in 4-7 is
nice because it lets us use BPF_OP. In this patchset wherever we have
(insn->imm == BPF_ADD) meaning "this is a traditional XADD without
fetch" and (BPF_OP(insn->imm) == BPF_ADD) meaning "this is an atomic
add, either with or without a fetch". 

Does that answer the question...?

> > diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> > index 0a721f6e8676..0767d7b579e9 100644
> > --- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> > +++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> > @@ -3109,13 +3109,19 @@ mem_xadd(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta, bool is64)
> >   	return 0;
> >   }
> > -static int mem_xadd4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> > +static int mem_atm4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> >   {
> > +	if (meta->insn.off != BPF_ADD)
> > +		return -EOPNOTSUPP;
> 
> meta->insn.imm?
> 
> > +
> >   	return mem_xadd(nfp_prog, meta, false);
> >   }
> > -static int mem_xadd8(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> > +static int mem_atm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> >   {
> > +	if (meta->insn.off != BPF_ADD)
> 
> meta->insn.imm?

Yikes, thanks for spotting these! Apparently I wasn't even compiling
this code.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  2020-11-24  6:50   ` Alexei Starovoitov
@ 2020-11-24 11:21     ` Brendan Jackman
  2020-11-24 22:43       ` Alexei Starovoitov
  0 siblings, 1 reply; 29+ messages in thread
From: Brendan Jackman @ 2020-11-24 11:21 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest

On Mon, Nov 23, 2020 at 10:50:04PM -0800, Alexei Starovoitov wrote:
> On Mon, Nov 23, 2020 at 05:31:58PM +0000, 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           | 27 +++++++++-------
> >  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                  | 26 +++++++++-------
> >  arch/sparc/net/bpf_jit_comp_64.c              | 14 +++++++--
> >  arch/x86/net/bpf_jit_comp.c                   | 30 +++++++++++-------
> >  arch/x86/net/bpf_jit_comp32.c                 |  6 ++--
> 
> I think this massive rename is not needed.
> BPF_XADD is uapi and won't be removed.
> Updating documentation, samples and tests is probably enough.

Ack, will tone down my agression against BPF_XADD! However the majority
of these changes are to various JITs, which do need to be updated, since
they need to check for nonzero immediate fields. Do you think I should
keep the renames where we're touching the code anyway?

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 7/7] bpf: Add tests for new BPF atomic operations
  2020-11-24  0:26   ` Yonghong Song
@ 2020-11-24 13:10     ` Brendan Jackman
  0 siblings, 0 replies; 29+ messages in thread
From: Brendan Jackman @ 2020-11-24 13:10 UTC (permalink / raw)
  To: Yonghong Song
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, KP Singh, Florent Revest

On Mon, Nov 23, 2020 at 04:26:45PM -0800, Yonghong Song wrote:
> On 11/23/20 9:32 AM, Brendan Jackman wrote:
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 3d5940cd110d..4e28640ca2d8 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -250,7 +250,7 @@ define CLANG_BPF_BUILD_RULE
> >   	$(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2)
> >   	$(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm			\
> >   		-c $1 -o - || echo "BPF obj compilation failed") | 	\
> > -	$(LLC) -mattr=dwarfris -march=bpf -mcpu=v3 $4 -filetype=obj -o $2
> > +	$(LLC) -mattr=dwarfris -march=bpf -mcpu=v4 $4 -filetype=obj -o $2
> 
> We have an issue here. If we change -mcpu=v4 here, we will force
> people to use trunk llvm to run selftests which is not a good idea.
> 
> I am wondering whether we can single out progs/atomics_test.c, which will be
> compiled with -mcpu=v4 and run with test_progs.
> 
> test_progs-no_alu32 runs tests without alu32. Since -mcpu=v4 implies
> alu32, atomic tests should be skipped in test_progs-no-alu32.
> 
> In bpf_helpers.h, we already use __clang_major__ macro to compare
> to clang version,
> 
> #if __clang_major__ >= 8 && defined(__bpf__)
> static __always_inline void
> bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
> {
>         if (!__builtin_constant_p(slot))
>                 __bpf_unreachable();
> ...
> 
> I think we could also use __clang_major__ in progs/atomics_test.c
> to enable tested intrinsics only if __clang_major__ >= 12? This
> way, the same code can be compiled with -mcpu=v2/v3.
> 
> Alternatively, you can also use a macro at clang command line like
>    clang -mcpu=v4 -DENABLE_ATOMIC ...
>    clang -mcpu=v3/v2 ...
> 
> progs/atomics_test.c:
>    #ifdef ENABLE_ATOMIC
>      ... atomic_intrinsics ...
>    #endif

Ah - all good points, thanks. Looks like tools/build/feature/ might
offer a solution here, I'll investigate.

> > diff --git a/tools/testing/selftests/bpf/progs/atomics_test.c b/tools/testing/selftests/bpf/progs/atomics_test.c
> > new file mode 100644
> > index 000000000000..d81f45eb6c45
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/atomics_test.c
> > @@ -0,0 +1,61 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +__u64 add64_value = 1;
> > +__u64 add64_result;
> > +__u32 add32_value = 1;
> > +__u32 add32_result;
> > +__u64 add_stack_value_copy;
> > +__u64 add_stack_result;
> 
> To please llvm10, let us initialize all unitialized globals explicitly like
>    __u64 add64_result = 0;
>    __u32 add32_result = 0;
>    ...
> 
> llvm11 and above are okay but llvm10 put those uninitialized globals
> into COM section (not .bss or .data sections) which BTF did not
> handle them.

Thanks, will initialise everything explicitly.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  2020-11-24 11:02     ` Brendan Jackman
@ 2020-11-24 16:04       ` Yonghong Song
  0 siblings, 0 replies; 29+ messages in thread
From: Yonghong Song @ 2020-11-24 16:04 UTC (permalink / raw)
  To: Brendan Jackman
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, KP Singh, Florent Revest



On 11/24/20 3:02 AM, Brendan Jackman wrote:
> On Mon, Nov 23, 2020 at 03:54:38PM -0800, Yonghong Song wrote:
>>
>>
>> On 11/23/20 9:31 AM, Brendan Jackman wrote:
> ...
>>> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
>>> index 0207b6ea6e8a..897634d0a67c 100644
>>> --- a/arch/arm/net/bpf_jit_32.c
>>> +++ b/arch/arm/net/bpf_jit_32.c
>>> @@ -1620,10 +1620,9 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
>>>    		}
>>>    		emit_str_r(dst_lo, tmp2, off, ctx, BPF_SIZE(code));
>>>    		break;
>>> -	/* STX XADD: lock *(u32 *)(dst + off) += src */
>>> -	case BPF_STX | BPF_XADD | BPF_W:
>>> -	/* STX XADD: lock *(u64 *)(dst + off) += src */
>>> -	case BPF_STX | BPF_XADD | BPF_DW:
>>> +	/* Atomic ops */
>>> +	case BPF_STX | BPF_ATOMIC | BPF_W:
>>> +	case BPF_STX | BPF_ATOMIC | BPF_DW:
>>>    		goto notyet;
>>>    	/* STX: *(size *)(dst + off) = src */
>>>    	case BPF_STX | BPF_MEM | BPF_W:
>>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>>> index ef9f1d5e989d..f7b194878a99 100644
>>> --- a/arch/arm64/net/bpf_jit_comp.c
>>> +++ b/arch/arm64/net/bpf_jit_comp.c
>>> @@ -875,10 +875,18 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>>>    		}
>>>    		break;
>>> -	/* STX XADD: lock *(u32 *)(dst + off) += src */
>>> -	case BPF_STX | BPF_XADD | BPF_W:
>>> -	/* STX XADD: lock *(u64 *)(dst + off) += src */
>>> -	case BPF_STX | BPF_XADD | BPF_DW:
>>> +	case BPF_STX | BPF_ATOMIC | BPF_W:
>>> +	case BPF_STX | BPF_ATOMIC | BPF_DW:
>>> +		if (insn->imm != BPF_ADD) {
>>
>> Currently BPF_ADD (although it is 0) is encoded at bit 4-7 of imm.
>> Do you think we should encode it in 0-3 to make such a comparision
>> and subsequent insn->imm = BPF_ADD making more sense?
> 
> Sorry not quite sure what you mean by this... I think encoding in 4-7 is
> nice because it lets us use BPF_OP. In this patchset wherever we have
> (insn->imm == BPF_ADD) meaning "this is a traditional XADD without
> fetch" and (BPF_OP(insn->imm) == BPF_ADD) meaning "this is an atomic
> add, either with or without a fetch".
> 
> Does that answer the question...?

Yes, thanks for explanation. It is a little bit odd but certainly 
acceptable.

> 
>>> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
>>> index 0a721f6e8676..0767d7b579e9 100644
>>> --- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
>>> +++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
>>> @@ -3109,13 +3109,19 @@ mem_xadd(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta, bool is64)
>>>    	return 0;
>>>    }
>>> -static int mem_xadd4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>>> +static int mem_atm4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>>>    {
>>> +	if (meta->insn.off != BPF_ADD)
>>> +		return -EOPNOTSUPP;
>>
>> meta->insn.imm?
>>
>>> +
>>>    	return mem_xadd(nfp_prog, meta, false);
>>>    }
>>> -static int mem_xadd8(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>>> +static int mem_atm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>>>    {
>>> +	if (meta->insn.off != BPF_ADD)
>>
>> meta->insn.imm?
> 
> Yikes, thanks for spotting these! Apparently I wasn't even compiling
> this code.
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
  2020-11-24 11:21     ` Brendan Jackman
@ 2020-11-24 22:43       ` Alexei Starovoitov
  0 siblings, 0 replies; 29+ messages in thread
From: Alexei Starovoitov @ 2020-11-24 22:43 UTC (permalink / raw)
  To: Brendan Jackman
  Cc: bpf, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest

On Tue, Nov 24, 2020 at 3:21 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Mon, Nov 23, 2020 at 10:50:04PM -0800, Alexei Starovoitov wrote:
> > On Mon, Nov 23, 2020 at 05:31:58PM +0000, 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           | 27 +++++++++-------
> > >  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                  | 26 +++++++++-------
> > >  arch/sparc/net/bpf_jit_comp_64.c              | 14 +++++++--
> > >  arch/x86/net/bpf_jit_comp.c                   | 30 +++++++++++-------
> > >  arch/x86/net/bpf_jit_comp32.c                 |  6 ++--
> >
> > I think this massive rename is not needed.
> > BPF_XADD is uapi and won't be removed.
> > Updating documentation, samples and tests is probably enough.
>
> Ack, will tone down my agression against BPF_XADD! However the majority
> of these changes are to various JITs, which do need to be updated, since
> they need to check for nonzero immediate fields. Do you think I should
> keep the renames where we're touching the code anyway?

Ahh. The JITs need to check for imm==0 and enotsupp the rest?
Right. In such case updating all JITs at once is necessary.
I was hoping to avoid touching all of them to minimize the chance of
merge conflicts.
Looks like it's inevitable. I think it's fine then.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends
  2020-11-24 10:55     ` Brendan Jackman
@ 2020-11-24 22:51       ` Alexei Starovoitov
  0 siblings, 0 replies; 29+ messages in thread
From: Alexei Starovoitov @ 2020-11-24 22:51 UTC (permalink / raw)
  To: Brendan Jackman
  Cc: bpf, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
	KP Singh, Florent Revest

On Tue, Nov 24, 2020 at 2:55 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Mon, Nov 23, 2020 at 10:40:00PM -0800, Alexei Starovoitov wrote:
> > On Mon, Nov 23, 2020 at 05:32:01PM +0000, Brendan Jackman wrote:
> > > These are the operations that implement atomic exchange and
> > > compare-exchange.
> > >
> > > They are peculiarly named because of the presence of the separate
> > > FETCH field that tells you whether the instruction writes the value
> > > back to the src register. Neither operation is supported without
> > > BPF_FETCH:
> > >
> > > - BPF_CMPSET without BPF_FETCH (i.e. an atomic compare-and-set
> > >   without knowing whether the write was successfully) isn't implemented
> > >   by the kernel, x86, or ARM. It would be a burden on the JIT and it's
> > >   hard to imagine a use for this operation, so it's not supported.
> > >
> > > - BPF_SET without BPF_FETCH would be bpf_set, which has pretty
> > >   limited use: all it really lets you do is atomically set 64-bit
> > >   values on 32-bit CPUs. It doesn't imply any barriers.
> >
> > ...
> >
> > > -                   if (insn->imm & BPF_FETCH) {
> > > +                   switch (insn->imm) {
> > > +                   case BPF_SET | BPF_FETCH:
> > > +                           /* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
> > > +                           EMIT1(0x87);
> > > +                           break;
> > > +                   case BPF_CMPSET | BPF_FETCH:
> > > +                           /* r0 = atomic_cmpxchg(*(u32/u64*)(dst_reg + off), r0, src_reg); */
> > > +                           EMIT2(0x0F, 0xB1);
> > > +                           break;
> > ...
> > >  /* atomic op type fields (stored in immediate) */
> > > +#define BPF_SET            0xe0    /* atomic write */
> > > +#define BPF_CMPSET 0xf0    /* atomic compare-and-write */
> > > +
> > >  #define BPF_FETCH  0x01    /* fetch previous value into src reg */
> >
> > I think SET in the name looks odd.
> > I understand that you picked this name so that SET|FETCH together would form
> > more meaningful combination of words, but we're not planning to support SET
> > alone. There is no such instruction in a cpu. If we ever do test_and_set it
> > would be something different.
>
> Yeah this makes sense...
>
> > How about the following instead:
> > +#define BPF_XCHG     0xe1    /* atomic exchange */
> > +#define BPF_CMPXCHG  0xf1    /* atomic compare exchange */
> > In other words get that fetch bit right away into the encoding.
> > Then the switch statement above could be:
> > +                     switch (insn->imm) {
> > +                     case BPF_XCHG:
> > +                             /* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
> > +                             EMIT1(0x87);
> > ...
> > +                     case BPF_ADD | BPF_FETCH:
> > ...
> > +                     case BPF_ADD:
>
> ... Although I'm a little wary of this because it makes it very messy to
> do something like switch(BPF_OP(insn->imm)) since we'd have no name for
> BPF_OP(0xe1). That might be fine - I haven't needed such a construction
> so far (although I have used BPF_OP(insn->imm)) so maybe we wouldn't
> ever need it.
>
> What do you think? Maybe we add the `#define BPF_XCHG 0xe1` and then if we
> later need to do switch(BPF_OP(insn->imm)) we could bring back
> `#define BPF_SET 0xe` as needed?

I don't think we'll add C atomic_set any time soon.
Since kernel's atomic_set according to the kernel memory model is the
same as write_once.
Which is different from C atomic_set that is implemented in llvm as atomic_xchg
which includes the barrier. Kernel barriers are explicit.
I think eventually we may add various barriers to bpf isa, but not atomic_set.
Just like we don't add insns for read_once, write_once. The normal load/store
should be honored by JITs. So read_once/write_once == *(volatile uX *) in C
will be compiled by llvm into normal bpf ld/st and JITs have to
preserve them as-is.
Otherwise bpf memory model (when it's defined eventually) would have to
diverge too much from the kernel. I think it needs to preserve
read_once/write_once
just like the kernel. Hence no special C-like atomic_set and when JITs process
ld/st they have to emit them as single insn when possible.

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2020-11-24 22:52 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
2020-11-23 17:31 ` [PATCH 1/7] bpf: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
2020-11-23 17:31 ` [PATCH 2/7] bpf: x86: Factor out emission of REX byte Brendan Jackman
2020-11-23 17:31 ` [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
2020-11-23 23:54   ` Yonghong Song
2020-11-24 11:02     ` Brendan Jackman
2020-11-24 16:04       ` Yonghong Song
2020-11-24  3:28   ` kernel test robot
2020-11-24  3:28     ` kernel test robot
2020-11-24  6:50   ` Alexei Starovoitov
2020-11-24 11:21     ` Brendan Jackman
2020-11-24 22:43       ` Alexei Starovoitov
2020-11-23 17:31 ` [PATCH 4/7] bpf: Move BPF_STX reserved field check into BPF_STX verifier code Brendan Jackman
2020-11-23 17:32 ` [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
2020-11-23 21:12   ` kernel test robot
2020-11-23 21:12     ` kernel test robot
2020-11-23 21:51   ` kernel test robot
2020-11-23 21:51     ` kernel test robot
2020-11-24  6:52   ` Alexei Starovoitov
2020-11-24 10:48     ` Brendan Jackman
2020-11-23 17:32 ` [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends Brendan Jackman
2020-11-23 19:29   ` Brendan Jackman
2020-11-24  6:40   ` Alexei Starovoitov
2020-11-24 10:55     ` Brendan Jackman
2020-11-24 22:51       ` Alexei Starovoitov
2020-11-23 17:32 ` [PATCH 7/7] bpf: Add tests for new BPF atomic operations Brendan Jackman
2020-11-24  0:26   ` Yonghong Song
2020-11-24 13:10     ` Brendan Jackman
2020-11-23 17:36 ` [PATCH 0/7] Atomics for eBPF Brendan Jackman

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.