Skip to content

Commit

Permalink
Detect process id reuse
Browse files Browse the repository at this point in the history
Process IDs (PIDs) are reused on Linux, so it's possible that we start profiling
a Ruby process with some PID and later on, while still profiling, this
process exits and a new one is spawned with this same PID.

The profiling information will be wrong. rbperf has some guardrails to
help here, such as ensuring that method and path names are valid
unicode, otherwise we consider the stack to be invalid, but it would be
best to actively ensure we don't have a race condition here.

By using the `pid + start_time` of the process, we should be able to have
a truly unique identifier of every process

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
  • Loading branch information
javierhonduco committed Sep 4, 2022
1 parent 7c14cd0 commit 272c8f3
Show file tree
Hide file tree
Showing 8 changed files with 136,842 additions and 256 deletions.
1 change: 1 addition & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
src/bpf/vmlinux.h -linguist-generated
17 changes: 17 additions & 0 deletions src/bpf/basic_types.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
typedef signed char __s8;
typedef unsigned char __u8;
typedef short int __s16;
typedef short unsigned int __u16;
typedef int __s32;
typedef unsigned int __u32;
typedef long long int __s64;
typedef long long unsigned int __u64;

typedef __s8 s8;
typedef __u8 u8;
typedef __s16 s16;
typedef __u16 u16;
typedef __s32 s32;
typedef __u32 u32;
typedef __s64 s64;
typedef __u64 u64;
42 changes: 38 additions & 4 deletions src/bpf/rbperf.bpf.c
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,15 @@
//
// Copyright (c) 2022 The rbperf authors

// clang-format off
#include "rbperf.h"

#include "vmlinux.h"

#include <bpf/bpf_core_read.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
// clang-format on

struct {
// This map's type is a placeholder, it's dynamically set
Expand Down Expand Up @@ -68,6 +72,7 @@ struct {

const volatile bool verbose = false;
const volatile bool use_ringbuf = false;
const volatile bool enable_pid_race_detector = true;
const volatile enum rbperf_event_type event_type = RBPERF_EVENT_SYSCALL_UNKNOWN;

#define LOG(fmt, ...) \
Expand All @@ -77,8 +82,7 @@ const volatile enum rbperf_event_type event_type = RBPERF_EVENT_SYSCALL_UNKNOWN;
} \
})


static inline_method int read_syscall_id(void* ctx, int* syscall_id) {
static inline_method int read_syscall_id(void *ctx, int *syscall_id) {
return bpf_probe_read_kernel(syscall_id, SYSCALL_NR_SIZE, ctx + SYSCALL_NR_OFFSET);
}

Expand Down Expand Up @@ -166,7 +170,7 @@ read_frame(u64 pc, u64 body, RubyFrame *current_frame,
LOG("[debug] reading stack");

rbperf_read(&path_addr, 8,
(void *)(body + location_offset + path_offset));
(void *)(body + ruby_location_offset + path_offset));
rbperf_read(&flags, 8, (void *)path_addr);
if ((flags & RUBY_T_MASK) == RUBY_T_STRING) {
path = path_addr;
Expand All @@ -186,7 +190,7 @@ read_frame(u64 pc, u64 body, RubyFrame *current_frame,
}

rbperf_read(&label, 8,
(void *)(body + location_offset + label_offset));
(void *)(body + ruby_location_offset + label_offset));

read_ruby_string(path, current_frame->path, sizeof(current_frame->path));
current_frame->lineno = read_ruby_lineno(pc, body, version_offsets);
Expand Down Expand Up @@ -323,6 +327,36 @@ int on_event(struct bpf_perf_event_data *ctx) {
if (process_data != NULL && process_data->rb_frame_addr != 0) {
LOG("[debug] reading Ruby stack");

struct task_struct *task = (void *)bpf_get_current_task();
if (task == NULL) {
LOG("[error] task_struct was NULL");
return 0;
}

// PIDs in Linux are reused. To ensure that the process we are
// profiling is the one we expect, we check the pid + start_time
// of the process.
//
// When we start profiling, the start_time will be zero, so we set
// it to the actual start time. Otherwise, we check that the start_time
// of the process matches what we expect. If it's not the case, bail out
// early, to avoid profiling the wrong process.
if (enable_pid_race_detector) {
u64 process_start_time;
bpf_core_read(&process_start_time, 8, &task->start_time);

if (process_data->start_time == 0) {
// First time seeing this process
process_data->start_time = process_start_time;
} else {
// Let's check that the start time matches what we saw before
if (process_data->start_time != process_start_time) {
LOG("[error] the process has probably changed...");
return 0;
}
}
}

u64 ruby_current_thread_addr;
u64 main_thread_addr;
u64 ec_addr;
Expand Down
5 changes: 3 additions & 2 deletions src/bpf/rbperf.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// This source code is licensed under the MIT license found in the
// LICENSE file in the root directory of this source tree.

#include "vmlinux.h"
#include "basic_types.h"

#define COMM_MAXLEN 25
#define METHOD_MAXLEN 50
Expand All @@ -28,7 +28,7 @@

#define iseq_offset 0x10 // offsetof(rb_control_frame_t, iseq)
#define body_offset 0x10 // offsetof(struct rb_iseq_struct, body)
#define location_offset 0x40 // offsetof(struct rb_iseq_constant_body, location)
#define ruby_location_offset 0x40 // offsetof(struct rb_iseq_constant_body, location)
#define path_offset 0x0 // offsetof(struct rb_iseq_location_struct, path)
#define iseq_encoded_offset \
0x8 // offsetof(struct rb_iseq_constant_body, iseq_encoded)
Expand Down Expand Up @@ -97,6 +97,7 @@ typedef struct {
typedef struct {
u64 rb_frame_addr;
int rb_version;
u64 start_time;
} ProcessData;

typedef struct {
Expand Down
Loading

0 comments on commit 272c8f3

Please sign in to comment.