Skip to content

Commit

Permalink
Fix atomic alu with fetch (vbpf#546)
Browse files Browse the repository at this point in the history 
* Handle case where src == R0 in emit_atomic_fetch_alu

Signed-off-by: Alan Jowett <[email protected]>

* Fix ubpf dissassembler

Signed-off-by: Alan Jowett <[email protected]>

* Reject exchange and compare exchange without fetch flag

Signed-off-by: Alan Jowett <[email protected]>

* Use correct __sync operation

Signed-off-by: Alan Jowett <[email protected]>

* Revert compare exchange intrinsic change

Signed-off-by: Alan Jowett <[email protected]>

* Update disassembler.py

* Update vm/ubpf_vm.c

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

---------

Signed-off-by: Alan Jowett <[email protected]>
Signed-off-by: Alan Jowett <[email protected]>
Co-authored-by: Alan Jowett <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
  • Loading branch information
@Alan-Jowett @coderabbitai
3 people authored Oct 8, 2024
1 parent 10851a4 commit a25446f
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 14 deletions.
39 changes: 27 additions & 12 deletions vm/ubpf_jit_x86_64.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,33 +835,52 @@ emit_atomic_fetch_alu(struct jit_state* state, int is_64bit, int opcode, int src
// x64 lacks a 64-bit atomic version of some alu-fetch instruction, so we emulate it with a compare-exchange.
// This is not a problem because the compare-exchange instruction is a full memory barrier.

// Compare exchange overwrites RAX, so we need to save it.
emit_push(state, RAX);
// The atomic compare exchange instruction overwrites RAX. If RAX is the source register, then save the original
// value in R10 or R11, depending on which one is not the destination.
int actual_src = src == RAX ? (dst == R10 ? R11 : R10) : src;

if (src != RAX) {
// Compare exchange overwrites RAX, so we need to save it.
emit_push(state, RAX);
} else {
// Save the original value in actual_src (r10 or r11).
emit_push(state, actual_src);

// Move src into actual_src.
emit_mov(state, src, actual_src);
}

// Load the original value at the destination into RAX.
emit_load(state, is_64bit ? S64 : S32, dst, RAX, offset);

// Loop until we successfully update the value.
uint32_t loop_start = state->offset;

// Copy RAX to RCX
// Copy RAX to RCX (required to preserve the original value of RAX for the comparison).
emit_mov(state, RAX, RCX);

// Perform the ALU operation into RCX.
emit_alu64(state, opcode, src, RCX);
emit_alu64(state, opcode, actual_src, RCX);

// Attempt to compare-exchange the value.
// Atomic compare exchange compares the value at [dst + offset] with RAX and if they are equal, it stores RCX into
// [dst + offset]. It always store the original value at [dst + offset] into RAX.
emit_atomic_cmp_exch_with_rax(state, is_64bit, RCX, dst, offset);

// If the compare-exchange failed, loop.
emit1(state, 0x75);
emit1(state, loop_start - state->offset - 1);

// Move RAX into the src register
emit_mov(state, RAX, src);
if (src != RAX) {
// Move RAX into the src register.
emit_mov(state, RAX, src);

// Restore RAX
emit_pop(state, RAX);
// Restore RAX
emit_pop(state, RAX);
} else {
// Restore the original value of actual_src.
emit_pop(state, actual_src);
}
}

static inline void
Expand Down Expand Up @@ -912,7 +931,6 @@ emit_atomic_xor32(struct jit_state* state, int src, int dst, int offset)
emit_atomic_alu(state, X64_ALU_XOR, IS_32BIT, src, dst, offset);
}


static inline void
emit_atomic_compare_exchange64(struct jit_state* state, int src, int dst, int offset)
{
Expand All @@ -925,7 +943,6 @@ emit_atomic_compare_exchange32(struct jit_state* state, int src, int dst, int of
emit_atomic_cmp_exch_with_rax(state, IS_32BIT, src, dst, offset);
}


static inline void
emit_atomic_exchange64(struct jit_state* state, int src, int dst, int offset)
{
Expand Down Expand Up @@ -1984,8 +2001,6 @@ translate(struct ubpf_vm* vm, struct jit_state* state, char** errmsg)
return 0;
}



static bool
resolve_patchable_relatives(struct jit_state* state)
{
Expand Down
21 changes: 19 additions & 2 deletions vm/ubpf_vm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1300,7 +1300,8 @@ ubpf_exec_ex(

case EBPF_OP_ATOMIC32_STORE: {
BOUNDS_CHECK_STORE(4);
bool fetch = inst.imm & EBPF_ATOMIC_OP_FETCH;
bool fetch = (inst.imm & EBPF_ATOMIC_OP_FETCH) || (inst.imm == EBPF_ATOMIC_OP_CMPXCHG) ||
(inst.imm == EBPF_ATOMIC_OP_XCHG);
// If this is a fetch instruction, the destination register is used to store the result.
int fetch_index = inst.src;
volatile uint32_t* destination = (volatile uint32_t*)(reg[inst.dst] + inst.offset);
Expand Down Expand Up @@ -1595,8 +1596,16 @@ validate(const struct ubpf_vm* vm, const struct ebpf_inst* insts, uint32_t num_i
case EBPF_ALU_OP_XOR:
break;
case (EBPF_ATOMIC_OP_XCHG & ~EBPF_ATOMIC_OP_FETCH):
if (!(inst.imm & EBPF_ATOMIC_OP_FETCH)) {
*errmsg = ubpf_error("invalid atomic operation at PC %d", i);
return false;
}
break;
case (EBPF_ATOMIC_OP_CMPXCHG & ~EBPF_ATOMIC_OP_FETCH):
if (!(inst.imm & EBPF_ATOMIC_OP_FETCH)) {
*errmsg = ubpf_error("invalid atomic operation at PC %d", i);
return false;
}
break;
default:
*errmsg = ubpf_error("invalid atomic operation at PC %d", i);
Expand All @@ -1617,8 +1626,16 @@ validate(const struct ubpf_vm* vm, const struct ebpf_inst* insts, uint32_t num_i
case EBPF_ALU_OP_XOR:
break;
case (EBPF_ATOMIC_OP_XCHG & ~EBPF_ATOMIC_OP_FETCH):
if (!(inst.imm & EBPF_ATOMIC_OP_FETCH)) {
*errmsg = ubpf_error("invalid atomic operation at PC %d", i);
return false;
}
break;
case (EBPF_ATOMIC_OP_CMPXCHG & ~EBPF_ATOMIC_OP_FETCH):
case (EBPF_ATOMIC_OP_CMPXCHG & ~EBPF_ATOMIC_OP_FETCH):
if (!(inst.imm & EBPF_ATOMIC_OP_FETCH)) {
*errmsg = ubpf_error("invalid atomic operation at PC %d", i);
return false;
}
break;
default:
*errmsg = ubpf_error("invalid atomic operation with opcode 0x%02x at PC %d", inst.opcode, i);
Expand Down

0 comments on commit a25446f

Please sign in to comment.