Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce load-acquire and store-release BPF instructions #4915

Closed
wants to merge 9 commits into from
Closed

Introduce load-acquire and store-release BPF instructions #4915

wants to merge 9 commits into from
+1,164 −105

Conversation

kernel-patches-daemon-bpf-rc[bot]
Copy link

Pull request for series with
subject: Introduce load-acquire and store-release BPF instructions
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: b420b57
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: b420b57
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 0fc5ddd
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c03320a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c03320a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c03320a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 57e71f8
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 9af5c78
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 5b67071
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 03f3aa4
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=928281
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 0abff46
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=931413
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 003be25
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=931413
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 003be25
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=931413
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: ea145d5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=931413
version: 2

@peilin-ye
bpf/verifier: Factor out atomic_ptr_type_ok()
888219a 
Factor out atomic_ptr_type_ok() as a helper function to be used later.

Signed-off-by: Peilin Ye <[email protected]>
@peilin-ye
bpf/verifier: Factor out check_atomic_rmw()
f241472 
Currently, check_atomic() only handles atomic read-modify-write (RMW)
instructions.  Since we are planning to introduce other types of atomic
instructions (i.e., atomic load/store), extract the existing RMW
handling logic into its own function named check_atomic_rmw().

Remove the @insn_idx parameter as it is not really necessary.  Use
'env->insn_idx' instead, as in other places in verifier.c.

Signed-off-by: Peilin Ye <[email protected]>
@peilin-ye
bpf/verifier: Factor out check_load_mem() and check_store_reg()
4fce722 
Extract BPF_LDX and most non-atomic BPF_STX instruction handling logic
in do_check() into helper functions to be used later.  While we are
here, make that comment about "reserved fields" more specific.

Suggested-by: Eduard Zingerman <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
@peilin-ye
bpf: Introduce load-acquire and store-release instructions
7891975 
Introduce BPF instructions with load-acquire and store-release
semantics, as discussed in [1].  The following new flags are defined:

  BPF_ATOMIC_LOAD         0x10
  BPF_ATOMIC_STORE        0x20
  BPF_ATOMIC_TYPE(imm)    ((imm) & 0xf0)

  BPF_RELAXED        0x0
  BPF_ACQUIRE        0x1
  BPF_RELEASE        0x2
  BPF_ACQ_REL        0x3
  BPF_SEQ_CST        0x4

  BPF_LOAD_ACQ       (BPF_ATOMIC_LOAD | BPF_ACQUIRE)
  BPF_STORE_REL      (BPF_ATOMIC_STORE | BPF_RELEASE)

A "load-acquire" is a BPF_STX | BPF_ATOMIC instruction with the 'imm'
field set to BPF_LOAD_ACQ (0x11).

Similarly, a "store-release" is a BPF_STX | BPF_ATOMIC instruction with
the 'imm' field set to BPF_STORE_REL (0x22).

Unlike existing atomic operations that only support BPF_W (32-bit) and
BPF_DW (64-bit) size modifiers, load-acquires and store-releases also
support BPF_B (8-bit) and BPF_H (16-bit).  An 8- or 16-bit load-acquire
zero-extends the value before writing it to a 32-bit register, just like
ARM64 instruction LDARH and friends.

As an example, consider the following 64-bit load-acquire BPF
instruction:

  db 10 00 00 11 00 00 00  r0 = load_acquire((u64 *)(r1 + 0x0))

  opcode (0xdb): BPF_ATOMIC | BPF_DW | BPF_STX
  imm (0x00000011): BPF_LOAD_ACQ

Similarly, a 16-bit BPF store-release:

  cb 21 00 00 22 00 00 00  store_release((u16 *)(r1 + 0x0), w2)

  opcode (0xcb): BPF_ATOMIC | BPF_H | BPF_STX
  imm (0x00000022): BPF_STORE_REL

In arch/{arm64,s390,x86}/net/bpf_jit_comp.c, have
bpf_jit_supports_insn(..., /*in_arena=*/true) return false for the new
instructions, until the corresponding JIT compiler supports them.

[1] https://lore.kernel.org/all/[email protected]/

Acked-by: Eduard Zingerman <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
Acked-by: Ilya Leoshkevich <[email protected]>
@peilin-ye
arm64: insn: Add BIT(23) to {load,store}_ex's mask
91a3e16 
We are planning to add load-acquire (LDAR{,B,H}) and store-release
(STLR{,B,H}) instructions to insn.{c,h}; add BIT(23) to mask of load_ex
and store_ex to prevent aarch64_insn_is_{load,store}_ex() from returning
false-positives for load-acquire and store-release instructions.

Reference: Arm Architecture Reference Manual (ARM DDI 0487K.a,
           ID032224),

  * C6.2.228 LDXR
  * C6.2.165 LDAXR
  * C6.2.161 LDAR
  * C6.2.393 STXR
  * C6.2.360 STLXR
  * C6.2.353 STLR

Signed-off-by: Peilin Ye <[email protected]>
@peilin-ye
arm64: insn: Add load-acquire and store-release instructions
293f2d0 
Add load-acquire ("load_acq", LDAR{,B,H}) and store-release
("store_rel", STLR{,B,H}) instructions.  Breakdown of encoding:

                                size        L   (Rs)  o0 (Rt2) Rn    Rt
             mask (0x3fdffc00): 00 111111 1 1 0 11111 1  11111 00000 00000
  value, load_acq (0x08dffc00): 00 001000 1 1 0 11111 1  11111 00000 00000
 value, store_rel (0x089ffc00): 00 001000 1 0 0 11111 1  11111 00000 00000

As suggested by Xu [1], include all Should-Be-One (SBO) bits ("Rs" and
"Rt2" fields) in the "mask" and "value" numbers.

It is worth noting that we are adding the "no offset" variant of STLR
instead of the "pre-index" variant, which has a different encoding.

Reference: Arm Architecture Reference Manual (ARM DDI 0487K.a,
           ID032224),

  * C6.2.161 LDAR
  * C6.2.353 STLR

[1] https://lore.kernel.org/bpf/[email protected]/

Signed-off-by: Peilin Ye <[email protected]>
@peilin-ye
bpf, arm64: Support load-acquire and store-release instructions
8ef1cd8 
Support BPF load-acquire (BPF_LOAD_ACQ) and store-release
(BPF_STORE_REL) instructions in the arm64 JIT compiler.  For example:

  db 10 00 00 11 00 00 00  r0 = load_acquire((u64 *)(r1 + 0x0))
  95 00 00 00 00 00 00 00  exit

  opcode (0xdb): BPF_ATOMIC | BPF_DW | BPF_STX
  imm (0x00000011): BPF_LOAD_ACQ

The JIT compiler would emit an LDAR instruction for the above, e.g.:

  ldar  x7, [x0]

Similarly, consider the following 16-bit store-release:

  cb 21 00 00 22 00 00 00  store_release((u16 *)(r1 + 0x0), w2)
  95 00 00 00 00 00 00 00  exit

  opcode (0xcb): BPF_ATOMIC | BPF_H | BPF_STX
  imm (0x00000022): BPF_ATOMIC_STORE | BPF_RELEASE

An STLRH instruction would be emitted, e.g.:

  stlrh  w1, [x0]

For a complete mapping:

  load-acquire     8-bit  LDARB
 (BPF_LOAD_ACQ)   16-bit  LDARH
                  32-bit  LDAR (32-bit)
                  64-bit  LDAR (64-bit)
  store-release    8-bit  STLRB
 (BPF_STORE_REL)  16-bit  STLRH
                  32-bit  STLR (32-bit)
                  64-bit  STLR (64-bit)

Arena accesses are supported.
bpf_jit_supports_insn(..., /*in_arena=*/true) always returns true for
BPF_LOAD_ACQ and BPF_STORE_REL instructions, as they don't depend on
ARM64_HAS_LSE_ATOMICS.

Signed-off-by: Peilin Ye <[email protected]>
@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 9b6cdaf
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=931413
version: 2

@peilin-ye
selftests/bpf: Add selftests for load-acquire and store-release instr…
82a2d1b 
…uctions

Add several ./test_progs tests:

  - arena_atomics/load_acquire
  - arena_atomics/store_release
  - verifier_load_acquire/*
  - verifier_store_release/*
  - verifier_precision/bpf_load_acquire
  - verifier_precision/bpf_store_release

The last two tests are added to check if backtrack_insn() handles the
new instructions correctly.

Additionally, the last test also makes sure that the verifier
"remembers" the value (in src_reg) we store-release into e.g. a stack
slot.  For example, if we take a look at the test program:

    #0:  r1 = 8;
      /* store_release((u64 *)(r10 - 8), r1); */
    #1:  .8byte %[store_release];
    #2:  r1 = *(u64 *)(r10 - 8);
    #3:  r2 = r10;
    #4:  r2 += r1;
    #5:  r0 = 0;
    #6:  exit;

At #1, if the verifier doesn't remember that we wrote 8 to the stack,
then later at #4 we would be adding an unbounded scalar value to the
stack pointer, which would cause the program to be rejected:

  VERIFIER LOG:
  =============
...
  math between fp pointer and register with unbounded min value is not allowed

All new tests depend on #ifdef ENABLE_ATOMICS_TESTS.  Currently they
only run for arm64.

Signed-off-by: Peilin Ye <[email protected]>
@peilin-ye
bpf, docs: Update instruction-set.rst for load-acquire and store-rele…
face973 
…ase instructions

Update documentation for the new load-acquire and store-release
instructions.  Rename existing atomic operations as "atomic
read-modify-write (RMW) operations".

Following RFC 9669, section 7.3. "Adding Instructions", create new
conformance groups "atomic32v2" and "atomic64v2", where:

  * atomic32v2: includes all instructions in "atomic32", plus the new
                8-bit, 16-bit and 32-bit atomic load-acquire and
                store-release instructions

  * atomic64v2: includes all instructions in "atomic64" and
                "atomic32v2", plus the new 64-bit atomic load-acquire
                and store-release instructions

Cc: [email protected]
Signed-off-by: Peilin Ye <[email protected]>
@kernel-patches-daemon-bpf-rc
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=931413 expired. Closing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bpf-next changes-requested V2-ci-fail V2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@peilin-ye