Skip to content
This repository has been archived by the owner on Jan 10, 2025. It is now read-only.

Fix - JIT ExceededMaxInstructions and ExecutionOverrun #591

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Fix - JIT ExceededMaxInstructions and ExecutionOverrun #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01aa047
Fixes missing pc reporting in JIT when throwing EbpfError::ExecutionO…
Lichtso Sep 12, 2024
15b5bcd
Adds missing test coverage for EbpfError::ExecutionOverrun.
Lichtso Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/jit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,8 +731,9 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> {
// Bumper in case there was no final exit
if self.offset_in_text_section + MAX_MACHINE_CODE_LENGTH_PER_INSTRUCTION * 2 >= self.result.text_section.len() {
return Err(EbpfError::ExhaustedTextSegment(self.pc));
}
self.emit_validate_and_profile_instruction_count(true, false, Some(self.pc + 2));
}
self.emit_validate_and_profile_instruction_count(true, false, Some(self.pc + 1));
self.emit_ins(X86Instruction::load_immediate(OperandSize::S64, REGISTER_SCRATCH, self.pc as i64)); // Save pc
self.emit_set_exception_kind(EbpfError::ExecutionOverrun);
self.emit_ins(X86Instruction::jump_immediate(self.relative_to_anchor(ANCHOR_THROW_EXCEPTION, 5)));

Expand Down
35 changes: 35 additions & 0 deletions tests/execution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3453,12 +3453,47 @@ fn test_err_fixed_stack_out_of_bound() {
);
}

#[test]
fn test_execution_overrun() {
let config = Config {
enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1,
..Config::default()
};
test_interpreter_and_jit_asm!(
"
add r1, 0",
config.clone(),
[],
(),
TestContextObject::new(2),
ProgramResult::Err(EbpfError::ExecutionOverrun),
);
test_interpreter_and_jit_asm!(
"
add r1, 0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this exceed maximum instructions while the lddw case below does not? Add is one instruction while lddw is two.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, Lddw accounts for a single instruction, not two. Let me reformulate my question. Why a does single instruction raise the ExceededMaxInstructions when we only request one unit for execution?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lddw only costs one CU. And this tests add, not lddw here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lddw only costs one CU.

I wrote exactly that in my previous comment:

Ah, Lddw accounts for a single instruction, not two

There is no need to repeat what I said. You've probably missed my comment. Now, my question is:

Why a does single instruction raise the ExceededMaxInstructions when we only request one unit for execution?

So in the add test, we only have a single add instruction and request one CU, but we raise an ExceededMaxInstructions error. Why is this the case?

And this tests add, not lddw here.

I didn't understand how this answers my question.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the add test, we only have a single add instruction and request one CU, but we raise an ExceededMaxInstructions error. Why is this the case?

I suppose logically, the ExceededMaxInstructions and ExecutionOverrun faults happen after the same time here, and it's not clear what to return.
After add is executed, the virtual CPU is in a state where it expects another instruction. But at this point CU==0 and PC is OOB, so the next instruction would fault for either of these reasons.

I suppose you could specify that instruction fetch occurs before CU checks, but that's just going to inhibit future possible optimizations.

config.clone(),
[],
(),
TestContextObject::new(1),
ProgramResult::Err(EbpfError::ExceededMaxInstructions),
);
}

#[test]
fn test_lddw() {
let config = Config {
enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1,
..Config::default()
};
test_interpreter_and_jit_asm!(
"
lddw r0, 0x1122334455667788",
config.clone(),
[],
(),
TestContextObject::new(2),
ProgramResult::Err(EbpfError::ExecutionOverrun),
);
test_interpreter_and_jit_asm!(
"
lddw r0, 0x1122334455667788
Expand Down