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.

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

Fix atomic alu with fetch #546

Merged
merged 11 commits into from
Oct 8, 2024

Conversation

Alan-Jowett
Copy link
Collaborator

This pull request introduces support for atomic operations in the disassembler and improves the handling of atomic compare-exchange instructions in the JIT compiler. The most important changes include handling atomic operations in the disassembler, saving the original value of the source register in the JIT compiler, and ensuring atomic compare-exchange instructions are correctly implemented.

Disassembler Enhancements:

  • Added support for atomic operations in the disassemble_one function in ubpf/disassembler.py. This includes handling various atomic opcodes and generating appropriate assembly instructions.

JIT Compiler Improvements:

  • Updated emit_atomic_fetch_alu in vm/ubpf_jit_x86_64.c to handle cases where the source register is RAX by saving the original value in either R10 or R11. This ensures the atomic compare-exchange instruction does not overwrite the source register.
  • Modified emit_atomic_xor32 and emit_atomic_compare_exchange32 to remove unnecessary blank lines, improving code readability. [1] [2]
  • Cleaned up the translate function by removing extra blank lines for better code clarity.

@coveralls
Copy link

coveralls commented Sep 11, 2024

Coverage Status

coverage: 80.718% (-0.09%) from 80.809%
when pulling c69fade on Alan-Jowett:fix_atomic_alu_with_fetch
into 389cf54 on iovisor:main.

@Alan-Jowett
Copy link
Collaborator Author

Additional failure:

alanjo@alanjo-dev2:~/ubpf$ libfuzzer/split.sh artifacts/crash-7d9bcc963763cd1f80da6c09dedea38e1435c4e5
Extracting program-7d9bcc963763cd1f80da6c09dedea38e1435c4e5...
Extracting memory-7d9bcc963763cd1f80da6c09dedea38e1435c4e5...
Disassembling program-7d9bcc963763cd1f80da6c09dedea38e1435c4e5...
Program size: 32
Memory size: 9
Disassembled program:
mov %r0, 0x2900
sub32 %r0, 0x40309c74
lock xchg [%r1 + 0x0], %r0
exit
Memory contents:
00000000: 0000 0000 0001 1900 00                   .........
interpreter_result: bfcf8c8c
jit_result: 19010000000000

@Alan-Jowett
Copy link
Collaborator Author

alanjo@alanjo-dev2:~/ubpf$ xxd  artifacts/program-7d9bcc963763cd1f80da6c09dedea38e1435c4e5
00000000: b700 0000 0029 0000 1400 7800 749c 3040  .....)....x.t.0@
00000010: db01 0000 e600 ff1a 9500 fdff 5bb1 0001  ............[...

Invalid instruction opcode:
db + imm == e6.

This translates as EBPF_ATOMIC_OP_XCHG without the fetch bit set...

Signed-off-by: Alan Jowett <[email protected]>
@hawkinsw
Copy link
Collaborator

I am sorry for the delay! I have started to review but did not get as far as I would have liked! I will (I hope) finish up tomorrow!

@hawkinsw
Copy link
Collaborator

On a related note, what do you think about "sprucing up" the disassembler's output with some optional debugging and warning information? Given the program in artifacts/program-7d9bcc963763cd1f80da6c09dedea38e1435c4e5, you would see something like

$ source file.asm  | ./bin/ubpf-disassembler 
mov %r0, 0x2900
Details:
	Class: 0x7
	Regs: 0x0
	Offset: 0x0
	Immediate: 0x2900
	Warnings: 
-----------------
sub32 %r0, 0x40309c74
Details:
	Class: 0x4
	Regs: 0x0
	Offset: 0x78
	Immediate: 0x40309c74
	Warnings: off has a value but it is not used by the instruction.
-----------------
stxdw [%r1], %r0
Details:
	Class: 0x3
	Regs: 0x1
	Offset: 0x0
	Immediate: 0x1aff00e6
	Warnings: imm has a value but it is not used by the instruction.
-----------------
exit
Details:
	Class: 0x5
	Regs: 0x0
	Offset: 0xfffd
	Immediate: 0x100b15b
	Warnings: off has a value but it is not used by the instruction; imm has a value but it is not used by the instruction.

@Alan-Jowett Alan-Jowett merged commit a25446f into iovisor:main Oct 8, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants