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

jit/x86: add emit_indirect_branch_target #288

Closed
wants to merge 1 commit into from

Conversation

riptl
Copy link

@riptl riptl commented Mar 21, 2022

This PR adds the ability to emit the endbr64 instruction to begin the work on Indirect Branch Tracking (IBT).
This does not introduce any functional changes to the JIT compiler.

The bulk of the work to enable IBT, which involves adding endbr64s to code translated from SBF, is not implemented in this PR.

Relates to #287

Changes

  • Adds test-only JitCompiler::new_mock() and JitCompiler::get_text_result() methods to allow test case assertions on x86-compiled code.
  • Add emit_indirect_branch_target function to emit endbr64 per Intel CET.

src/jit.rs Outdated Show resolved Hide resolved
@riptl riptl force-pushed the anchor-endbr64 branch 3 times, most recently from e36a585 to 279e7d9 Compare March 21, 2022 13:35
@@ -866,6 +867,11 @@ fn emit_set_exception_kind<E: UserDefinedError>(jit: &mut JitCompiler, err: Ebpf
X86Instruction::store_immediate(OperandSize::S64, R10, X86IndirectAccess::Offset(8), err_kind as i64).emit(jit)
}

#[inline]
fn emit_indirect_branch_target<E: UserDefinedError>(jit: &mut JitCompiler) -> Result<(), EbpfError<E>> {
X86Instruction::end_branch().emit(jit)
Copy link
Author

Choose a reason for hiding this comment

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

Should we put a feature flag here?

Copy link

Choose a reason for hiding this comment

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

Not necessary, in fact I would remove the entire emit_indirect_branch_target function and only let the X86Instruction::end_branch() in.

@Lichtso
Copy link

Lichtso commented Mar 21, 2022

Why put the indirect_branch_target in JitCompiler::set_anchor()?
Aren't all anchors targeted by direct jumps (instead of register indirect ones)?

Edit: I would expect indirect_branch_target to be placed at every instruction before the instruction tracer call.

@riptl
Copy link
Author

riptl commented Mar 21, 2022

I think I'm fundamentally misunderstanding call and return sites in the JIT. 😅 @Lichtso What do you think of just addressing a small PR first that adds the indirect_branch_target method without any uses instead? And then open a few small PRs for various uses of endbr64 whereever they are required?

@riptl riptl changed the title jit/x86: emit endbr64 on anchors jit/x86: add emit_indirect_branch_target Mar 21, 2022
@riptl
Copy link
Author

riptl commented Mar 21, 2022

Why put the indirect_branch_target in JitCompiler::set_anchor()? Aren't all anchors targeted by direct jumps (instead of register indirect ones)?

You're right. Updated PR to omit this change.

@@ -96,6 +98,11 @@ impl Default for X86Instruction {

impl X86Instruction {
pub fn emit<E: UserDefinedError>(&self, jit: &mut JitCompiler) -> Result<(), EbpfError<E>> {
match self.repeat_string {
Copy link

Choose a reason for hiding this comment

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

Maybe turn this into an enum:

  • Repeat Non Zero
  • Repeat Zero
  • Don't Repeat

@@ -866,6 +867,11 @@ fn emit_set_exception_kind<E: UserDefinedError>(jit: &mut JitCompiler, err: Ebpf
X86Instruction::store_immediate(OperandSize::S64, R10, X86IndirectAccess::Offset(8), err_kind as i64).emit(jit)
}

#[inline]
fn emit_indirect_branch_target<E: UserDefinedError>(jit: &mut JitCompiler) -> Result<(), EbpfError<E>> {
X86Instruction::end_branch().emit(jit)
Copy link

Choose a reason for hiding this comment

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

Not necessary, in fact I would remove the entire emit_indirect_branch_target function and only let the X86Instruction::end_branch() in.

@@ -980,6 +986,22 @@ impl JitCompiler {
})
}

#[cfg(test)]
#[allow(dead_code)]
pub(crate) fn new_mock() -> Self {
Copy link

Choose a reason for hiding this comment

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

Should be moved over in x86.rs mod test { ... } as this is only used by the instruction emitter.


#[cfg(test)]
#[allow(dead_code)]
pub(crate) fn get_text_result(&self) -> &[u8] {
Copy link

Choose a reason for hiding this comment

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

Probably not needed, just make the result accessible crate wide.

@Lichtso
Copy link

Lichtso commented Nov 14, 2022

Closing for now as we currently have no need for it and it got unresolved conflicts.

@Lichtso Lichtso closed this Nov 14, 2022
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.

2 participants