-
Notifications
You must be signed in to change notification settings - Fork 400
tracer/ruby: Optimize ep check to simplify work for verifier, increase limit #1100
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
Changes from all commits
696b8d0
c46b019
2462f24
bdbb195
937f7f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,15 +17,19 @@ struct ruby_procs_t { | |
| // The number of Ruby frames to unwind per frame-unwinding eBPF program. If | ||
| // we start running out of instructions in the walk_ruby_stack program, one | ||
| // option is to adjust this number downwards. | ||
| // NOTE the maximum size stack is this times 33 | ||
| // NOTE: the maximum size stack is FRAMES_PER_WALK_RUBY_STACK * calls to tail_call(). | ||
| #define FRAMES_PER_WALK_RUBY_STACK 32 | ||
| // When resolving a CME, we need to traverse environment pointers until we | ||
| // find IMEMO_MENT. Since we can't do a while loop, we have to bound this | ||
| // the max encountered in experimentation on a production rails app is 6. | ||
| // This increases insn for the kernel verifier all code in the ep check "loop" | ||
| // is M*N for instruction checks, so be extra sensitive about additions there. | ||
| // find IMEMO_MENT. Since we can't do a while loop, we have to bound this. | ||
| // The max encountered empirically on a production rails app is 6. | ||
| // This increases insn for the kernel verifier: all code in the ep check "loop" | ||
| // is M*N for instruction checks, so be extra sensitive about additions there. | ||
| // If we get ERR_RUBY_READ_CME_MAX_EP regularly, we may need to raise it. | ||
| #define MAX_EP_CHECKS 6 | ||
| #define MAX_EP_CHECKS 10 | ||
|
|
||
| // Constants related to reading a method entry | ||
| // https://github.com/ruby/ruby/blob/523857bfcb0f0cdfd1ed7faa09b9c59a0266e7e2/method.h#L118 | ||
|
|
@@ -40,6 +44,9 @@ struct ruby_procs_t { | |
| #define IMEMO_SVAR 2 | ||
| #define IMEMO_MENT 6 | ||
|
|
||
| // https://github.com/ruby/ruby/blob/36809a8d0c7ab67ff0919b331db926529a3e98a9/vm_core.h#L1375 | ||
| #define GC_GUARDED_PTR_REF_MASK 0x03 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed i had a magic constant below, named it and doc'd it here. |
||
|
|
||
| // https://github.com/ruby/ruby/blob/v3_4_5/vm_core.h#L1380-L1385 | ||
| #define VM_FRAME_MAGIC_MASK 0x7fff0001 | ||
| #define VM_FRAME_MAGIC_CFUNC 0x55550001 | ||
|
|
@@ -131,24 +138,23 @@ push_ruby(UnwindState *state, Trace *trace, u8 frame_type, u64 file, u64 line, u | |
| static EBPF_INLINE ErrorCode read_ruby_frame( | ||
| PerCPURecord *record, const RubyProcInfo *rubyinfo, void *stack_ptr, int *next_unwinder) | ||
| { | ||
| // Type of frame we found and are pushing (encoded in upper bits of Frame | ||
| u8 frame_type; | ||
| Trace *trace = &record->trace; | ||
| // Type of frame we found and are pushing | ||
| u8 frame_type = RUBY_FRAME_TYPE_NONE; | ||
| // Actual frame address of the given type | ||
| u64 frame_addr; | ||
| u64 frame_addr = 0; | ||
| // Address of the cfp->iseq, used to get the line number using the pc | ||
| u64 iseq_addr = 0; | ||
| u64 pc = 0; | ||
|
|
||
| Trace *trace = &record->trace; | ||
| u64 iseq_addr = 0; | ||
| u64 pc = 0; | ||
| // The maximum number of environment pointers to walk to find a 'local' env | ||
| u64 ep_check = 0; | ||
| u64 rbasic_flags = 0; | ||
| u64 imemo_mask = 0; | ||
| u64 me_or_cref = 0; | ||
| u64 svar_cref = 0; | ||
| void *current_ep = NULL; | ||
| u64 frame_flags = 0; | ||
| bool cfunc = false; | ||
|
|
||
| u64 ep_check = 0; | ||
| void *current_ep = NULL; | ||
|
|
||
| vm_env_t vm_env; | ||
| rb_control_frame_t control_frame; | ||
|
|
@@ -161,93 +167,98 @@ static EBPF_INLINE ErrorCode read_ruby_frame( | |
| current_ep = (void *)control_frame.ep; | ||
| pc = (u64)control_frame.pc; | ||
|
|
||
| // this code emulates ruby's rb_vm_frame_method_entry, which is called by | ||
| // Read the vm env from the 'base' ep | ||
| if (bpf_probe_read_user( | ||
| &vm_env, sizeof(vm_env), (void *)(current_ep - sizeof(vm_env) + sizeof(void *)))) { | ||
| DEBUG_PRINT("ruby: failed to get vm env"); | ||
| increment_metric(metricID_UnwindRubyErrReadEp); | ||
| return ERR_RUBY_READ_EP; | ||
| } | ||
|
dalehamel marked this conversation as resolved.
|
||
|
|
||
| // First method entry to check | ||
| me_or_cref = (u64)vm_env.me_cref; | ||
| // Check frame flags to see if it is a cfunc | ||
| frame_flags = (u64)vm_env.flags; | ||
| cfunc = (((frame_flags & VM_FRAME_MAGIC_MASK) == VM_FRAME_MAGIC_CFUNC) || pc == 0); | ||
|
|
||
| if (!cfunc) { | ||
| // Read the control frame iseq so we can get the line number | ||
| if (control_frame.iseq == NULL) { | ||
| increment_metric(metricID_UnwindRubyErrInvalidIseq); | ||
| return ERR_RUBY_INVALID_ISEQ; | ||
| } | ||
| if (bpf_probe_read_user( | ||
| &iseq_addr, sizeof(iseq_addr), (void *)(control_frame.iseq + rubyinfo->body))) { | ||
| increment_metric(metricID_UnwindRubyErrReadIseqBody); | ||
| return ERR_RUBY_READ_ISEQ_BODY; | ||
| } | ||
| } | ||
|
|
||
| // This code emulates ruby's rb_vm_frame_method_entry, which is called by | ||
| // rb_vm_frame_method_entry to check the frame for a callable method entry, CME | ||
| // If it cannot find a local method entry within MAX_EP_CHECKS, it will error | ||
| // https://github.com/ruby/ruby/blob/v3_4_7/vm_insnhelper.c#L769 | ||
| UNROLL for (ep_check = 0; ep_check < MAX_EP_CHECKS; ++ep_check) | ||
| { | ||
| // On every iteration except the first, get the ep from specval only if | ||
| // it is non-local. | ||
| if (ep_check > 0) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This weird check isn't necessary as we just do this processing before the loop starts now. |
||
| if (!((u64)vm_env.flags & VM_ENV_FLAG_LOCAL)) { | ||
| // https://github.com/ruby/ruby/blob/v3_4_5/vm_core.h#L1355 | ||
| current_ep = (void *)((u64)vm_env.specval & ~0x03); | ||
| } else { | ||
| break; | ||
| // This code emulates ruby's check_method_entry to traverse the environment | ||
| // until it finds a method entry. Since the function calls itself, the code | ||
| // is a bit out of order to try and optimize running as few instructions as | ||
| // possible, since this is in the M*N part of the loop and we need the code | ||
| // to pass the kernel verifier. | ||
| // https://github.com/ruby/ruby/blob/v3_4_7/vm_insnhelper.c#L743 | ||
| if (me_or_cref != 0) { | ||
| if (bpf_probe_read_user(&rbasic_flags, sizeof(rbasic_flags), (void *)(me_or_cref))) { | ||
| increment_metric(metricID_UnwindRubyErrReadRbasicFlags); | ||
| return ERR_RUBY_READ_RBASIC_FLAGS; | ||
| } | ||
|
|
||
| // https://github.com/ruby/ruby/blob/3361aa5c7df35b1d1daea578fefec3addf29c9a6/internal/imemo.h#L165-L169 | ||
| imemo_mask = (rbasic_flags >> RUBY_FL_USHIFT) & IMEMO_MASK; | ||
|
|
||
| // If the imemo is ever a method entry, we don't need to check further | ||
| if (imemo_mask == IMEMO_MENT) | ||
| break; | ||
| } | ||
|
|
||
| frame_addr = 0; | ||
| frame_type = RUBY_FRAME_TYPE_NONE; | ||
| cfunc = false; | ||
| // Only advance to checking the next EP if not local | ||
| if ((u64)vm_env.flags & VM_ENV_FLAG_LOCAL) | ||
| break; | ||
|
|
||
| // Mimic VM_ENV_PREV_EP | ||
| // https://github.com/ruby/ruby/blob/36809a8d0c7ab67ff0919b331db926529a3e98a9/vm_core.h#L1576 | ||
| current_ep = (void *)((u64)vm_env.specval & ~GC_GUARDED_PTR_REF_MASK); | ||
| if (bpf_probe_read_user( | ||
| &vm_env, sizeof(vm_env), (void *)(current_ep - sizeof(vm_env) + sizeof(void *)))) { | ||
| DEBUG_PRINT("ruby: failed to get vm env"); | ||
| increment_metric(metricID_UnwindRubyErrReadEp); | ||
| return ERR_RUBY_READ_EP; | ||
| } | ||
|
|
||
| // Prepare the value to check for the next iteration of the loop | ||
| me_or_cref = (u64)vm_env.me_cref; | ||
| // Only check the flags from the "root" env | ||
| if (frame_flags == 0) { | ||
| frame_flags = (u64)vm_env.flags; | ||
| } | ||
| cfunc = (((frame_flags & VM_FRAME_MAGIC_MASK) == VM_FRAME_MAGIC_CFUNC) || pc == 0); | ||
|
|
||
| if (!cfunc) { | ||
| // Read the control frame iseq so we can get the line number | ||
| if (control_frame.iseq == NULL) { | ||
| increment_metric(metricID_UnwindRubyErrInvalidIseq); | ||
| return ERR_RUBY_INVALID_ISEQ; | ||
| } | ||
| if (bpf_probe_read_user( | ||
| &iseq_addr, sizeof(iseq_addr), (void *)(control_frame.iseq + rubyinfo->body))) { | ||
| increment_metric(metricID_UnwindRubyErrReadIseqBody); | ||
| return ERR_RUBY_READ_ISEQ_BODY; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // this code emulate's ruby's check_method_entry to traverse the environment | ||
| // until it finds a method entry. Since the function calls itself, the code | ||
| // is a bit out of order to try and optimize running as few instructions as | ||
| // possible, since this is in the M * N part of the loop and we want the code | ||
| // to pass the kernel verifier. | ||
| // https://github.com/ruby/ruby/blob/v3_4_7/vm_insnhelper.c#L743 | ||
| if (me_or_cref == 0) | ||
| continue; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This continue statement was a headache. Now it just uses a branch and in the case it is 0, we advance to the next ep as that is the next code in the loop |
||
| // TODO: Perhaps rather than bailing on MAX_EP, we should push a dummy frame instead, | ||
| // so we can continue unwinding the stack. | ||
| if (ep_check >= MAX_EP_CHECKS) | ||
| return ERR_RUBY_READ_CME_MAX_EP; | ||
|
|
||
| if (bpf_probe_read_user(&rbasic_flags, sizeof(rbasic_flags), (void *)(me_or_cref))) { | ||
| increment_metric(metricID_UnwindRubyErrReadRbasicFlags); | ||
| return ERR_RUBY_READ_RBASIC_FLAGS; | ||
| } | ||
| // If the env is local, check if imemo is svar and if so, dereference it | ||
| if ((u64)vm_env.flags & VM_ENV_FLAG_LOCAL) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It turns out this doesn't need to run in the loop, as we only care about the base-case. an "svar" is only allowed once we have determined we are local, so we run that check here after the loop is done. The additional "SVAR" check lets is potentially dereference an imemo entry from the svar, and then we check that imemo in the next branch below. |
||
| if (imemo_mask == IMEMO_SVAR) { | ||
| if (bpf_probe_read_user(&svar_cref, sizeof(svar_cref), (void *)(me_or_cref + 8))) { | ||
| increment_metric(metricID_UnwindRubyErrReadSvar); | ||
| return ERR_RUBY_READ_SVAR; | ||
| } | ||
| me_or_cref = svar_cref; | ||
|
|
||
| // https://github.com/ruby/ruby/blob/3361aa5c7df35b1d1daea578fefec3addf29c9a6/internal/imemo.h#L165-L169 | ||
| imemo_mask = (rbasic_flags >> RUBY_FL_USHIFT) & IMEMO_MASK; | ||
|
|
||
| if ((u64)vm_env.flags & VM_ENV_FLAG_LOCAL) { | ||
| if (imemo_mask == IMEMO_SVAR) { | ||
| if (bpf_probe_read_user(&svar_cref, sizeof(svar_cref), (void *)(me_or_cref + 8))) { | ||
| increment_metric(metricID_UnwindRubyErrReadSvar); | ||
| return ERR_RUBY_READ_SVAR; | ||
| } | ||
| me_or_cref = svar_cref; | ||
|
|
||
| if (bpf_probe_read_user(&rbasic_flags, sizeof(rbasic_flags), (void *)(me_or_cref))) { | ||
| increment_metric(metricID_UnwindRubyErrReadRbasicFlags); | ||
| return ERR_RUBY_READ_RBASIC_FLAGS; | ||
| } | ||
| imemo_mask = (rbasic_flags >> RUBY_FL_USHIFT) & IMEMO_MASK; | ||
| if (bpf_probe_read_user(&rbasic_flags, sizeof(rbasic_flags), (void *)(me_or_cref))) { | ||
| increment_metric(metricID_UnwindRubyErrReadRbasicFlags); | ||
| return ERR_RUBY_READ_RBASIC_FLAGS; | ||
| } | ||
| imemo_mask = (rbasic_flags >> RUBY_FL_USHIFT) & IMEMO_MASK; | ||
| } | ||
|
|
||
| if (imemo_mask == IMEMO_MENT) | ||
| break; | ||
| } | ||
|
|
||
| if (ep_check >= MAX_EP_CHECKS) | ||
| return ERR_RUBY_READ_CME_MAX_EP; | ||
|
|
||
| if (imemo_mask == IMEMO_MENT) { | ||
| frame_addr = me_or_cref; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in the future if we do have issues with the verifier, this tunable and the one above for how many frames to walk can adjust this. However, I'd rather leave this at 10 as it should handle most cases, including more pathological workloads.