From c3eddb2e2effc01eee7bc778c21431bf45a8ffdc Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Fri, 19 Mar 2021 11:27:50 -0400 Subject: [PATCH] Fix: racy notifier captures update vs traversal For captures, a new struct lttng_kernel_notification_ctx is introduced, which is to be used as additional "context" to the notification_send() callback. This allows passing the "eval_capture" state from the probe to the notification callback, and fixes a bug where a sequence of: - create notification enabler, - enable notification enabler, - add capture to enabler, where a tracepoint runs concurrently with add capture happens to do a first capture list_empty check which skips the stack preparation, whereas the second capture list_empty check within the notification callback finds a capture entry, and thus attempts to use an uninitialized stack. The notification callback is also modified to use an RCU-aware list traversal. Signed-off-by: Mathieu Desnoyers Change-Id: Ib7ab673fb05b4824d0c7ba2e163dc1e88b465ee9 --- include/lttng/event-notifier-notification.h | 3 ++- include/lttng/events.h | 8 +++++++- include/lttng/tracepoint-event-impl.h | 18 ++++++++++++++---- src/lttng-event-notifier-notification.c | 7 ++++--- src/lttng-events.c | 2 ++ src/probes/lttng-kprobes.c | 4 +++- src/probes/lttng-uprobes.c | 4 +++- 7 files changed, 35 insertions(+), 11 deletions(-) diff --git a/include/lttng/event-notifier-notification.h b/include/lttng/event-notifier-notification.h index b044e2fa..160b2542 100644 --- a/include/lttng/event-notifier-notification.h +++ b/include/lttng/event-notifier-notification.h @@ -12,6 +12,7 @@ void lttng_event_notifier_notification_send(struct lttng_event_notifier *event_notifier, struct lttng_probe_ctx *lttng_probe_ctx, - const char *stack_data); + const char *stack_data, + struct lttng_kernel_notifier_ctx *notif_ctx); #endif /* _LTTNG_EVENT_NOTIFIER_NOTIFICATION_H */ diff --git a/include/lttng/events.h b/include/lttng/events.h index 6aa880f8..5d439bcb 100644 --- a/include/lttng/events.h +++ b/include/lttng/events.h @@ -353,6 +353,10 @@ struct lttng_event { int has_enablers_without_bytecode; }; +struct lttng_kernel_notifier_ctx { + int eval_capture; +}; + // FIXME: Really similar to lttng_event above. Could those be merged ? struct lttng_event_notifier { enum lttng_event_type evtype; /* First field. */ @@ -385,10 +389,12 @@ struct lttng_event_notifier { size_t num_captures; struct list_head capture_bytecode_runtime_head; int has_enablers_without_bytecode; + int eval_capture; /* Should evaluate capture */ void (*send_notification)(struct lttng_event_notifier *event_notifier, struct lttng_probe_ctx *lttng_probe_ctx, - const char *interpreter_stack_data); + const char *interpreter_stack_data, + struct lttng_kernel_notifier_ctx *notif_ctx); struct lttng_event_notifier_group *group; /* Weak ref */ }; diff --git a/include/lttng/tracepoint-event-impl.h b/include/lttng/tracepoint-event-impl.h index 079d7d37..f5464773 100644 --- a/include/lttng/tracepoint-event-impl.h +++ b/include/lttng/tracepoint-event-impl.h @@ -1430,6 +1430,8 @@ static void __event_notifier_probe__##_name(void *__data, _proto) \ struct probe_local_vars __tp_locvar; \ struct probe_local_vars *tp_locvar __attribute__((unused)) = \ &__tp_locvar; \ + struct lttng_kernel_notifier_ctx __notif_ctx; \ + bool __interpreter_stack_prepared = false; \ \ if (unlikely(!READ_ONCE(__event_notifier->enabled))) \ return; \ @@ -1440,6 +1442,7 @@ static void __event_notifier_probe__##_name(void *__data, _proto) \ \ __event_prepare_interpreter_stack__##_name(__stackvar.__interpreter_stack_data, \ tp_locvar, _args); \ + __interpreter_stack_prepared = true; \ lttng_list_for_each_entry_rcu(bc_runtime, &__event_notifier->filter_bytecode_runtime_head, node) { \ if (unlikely(bc_runtime->interpreter_funcs.filter(bc_runtime, &__lttng_probe_ctx, \ __stackvar.__interpreter_stack_data) & LTTNG_INTERPRETER_RECORD_FLAG)) \ @@ -1449,14 +1452,16 @@ static void __event_notifier_probe__##_name(void *__data, _proto) \ goto __post; \ } \ \ - if (unlikely(!list_empty(&__event_notifier->capture_bytecode_runtime_head))) \ + __notif_ctx.eval_capture = LTTNG_READ_ONCE(__event_notifier->eval_capture); \ + if (unlikely(!__interpreter_stack_prepared && __notif_ctx.eval_capture)) \ __event_prepare_interpreter_stack__##_name( \ __stackvar.__interpreter_stack_data, \ tp_locvar, _args); \ \ __event_notifier->send_notification(__event_notifier, \ &__lttng_probe_ctx, \ - __stackvar.__interpreter_stack_data); \ + __stackvar.__interpreter_stack_data, \ + &__notif_ctx); \ \ __post: \ _code_post \ @@ -1481,6 +1486,8 @@ static void __event_notifier_probe__##_name(void *__data) \ struct probe_local_vars __tp_locvar; \ struct probe_local_vars *tp_locvar __attribute__((unused)) = \ &__tp_locvar; \ + struct lttng_kernel_notifier_ctx __notif_ctx; \ + bool __interpreter_stack_prepared = false; \ \ if (unlikely(!READ_ONCE(__event_notifier->enabled))) \ return; \ @@ -1491,6 +1498,7 @@ static void __event_notifier_probe__##_name(void *__data) \ \ __event_prepare_interpreter_stack__##_name(__stackvar.__interpreter_stack_data, \ tp_locvar); \ + __interpreter_stack_prepared = true; \ lttng_list_for_each_entry_rcu(bc_runtime, &__event_notifier->filter_bytecode_runtime_head, node) { \ if (unlikely(bc_runtime->interpreter_funcs.filter(bc_runtime, &__lttng_probe_ctx, \ __stackvar.__interpreter_stack_data) & LTTNG_INTERPRETER_RECORD_FLAG)) \ @@ -1500,14 +1508,16 @@ static void __event_notifier_probe__##_name(void *__data) \ goto __post; \ } \ \ - if (unlikely(!list_empty(&__event_notifier->capture_bytecode_runtime_head))) \ + __notif_ctx.eval_capture = LTTNG_READ_ONCE(__event_notifier->eval_capture); \ + if (unlikely(!__interpreter_stack_prepared && __notif_ctx.eval_capture)) \ __event_prepare_interpreter_stack__##_name( \ __stackvar.__interpreter_stack_data, \ tp_locvar); \ \ __event_notifier->send_notification(__event_notifier, \ &__lttng_probe_ctx, \ - __stackvar.__interpreter_stack_data); \ + __stackvar.__interpreter_stack_data, \ + &__notif_ctx); \ __post: \ _code_post \ return; \ diff --git a/src/lttng-event-notifier-notification.c b/src/lttng-event-notifier-notification.c index 52b5593a..b39bfaad 100644 --- a/src/lttng-event-notifier-notification.c +++ b/src/lttng-event-notifier-notification.c @@ -426,7 +426,8 @@ void notification_send(struct lttng_event_notifier_notification *notif, void lttng_event_notifier_notification_send(struct lttng_event_notifier *event_notifier, struct lttng_probe_ctx *lttng_probe_ctx, - const char *stack_data) + const char *stack_data, + struct lttng_kernel_notifier_ctx *notif_ctx) { struct lttng_event_notifier_notification notif = { 0 }; int ret; @@ -440,7 +441,7 @@ void lttng_event_notifier_notification_send(struct lttng_event_notifier *event_n goto end; } - if (unlikely(!list_empty(&event_notifier->capture_bytecode_runtime_head))) { + if (unlikely(notif_ctx->eval_capture)) { struct lttng_bytecode_runtime *capture_bc_runtime; /* @@ -449,7 +450,7 @@ void lttng_event_notifier_notification_send(struct lttng_event_notifier *event_n * `output` parameter to the capture buffer. If the interpreter * fails, append an empty capture to the buffer. */ - list_for_each_entry(capture_bc_runtime, + list_for_each_entry_rcu(capture_bc_runtime, &event_notifier->capture_bytecode_runtime_head, node) { struct lttng_interpreter_output output; diff --git a/src/lttng-events.c b/src/lttng-events.c index d819c9e2..a144f544 100644 --- a/src/lttng-events.c +++ b/src/lttng-events.c @@ -2768,6 +2768,8 @@ void lttng_event_notifier_group_sync_enablers(struct lttng_event_notifier_group list_for_each_entry(runtime, &event_notifier->capture_bytecode_runtime_head, node) lttng_bytecode_capture_sync_state(runtime); + + WRITE_ONCE(event_notifier->eval_capture, !!event_notifier->num_captures); } } diff --git a/src/probes/lttng-kprobes.c b/src/probes/lttng-kprobes.c index 6824088c..dc18f507 100644 --- a/src/probes/lttng-kprobes.c +++ b/src/probes/lttng-kprobes.c @@ -54,11 +54,13 @@ int lttng_kprobes_event_notifier_handler_pre(struct kprobe *p, struct pt_regs *r { struct lttng_event_notifier *event_notifier = container_of(p, struct lttng_event_notifier, u.kprobe.kp); + struct lttng_kernel_notifier_ctx notif_ctx; if (unlikely(!READ_ONCE(event_notifier->enabled))) return 0; - event_notifier->send_notification(event_notifier, NULL, NULL); + notif_ctx.eval_capture = LTTNG_READ_ONCE(event_notifier->eval_capture); + event_notifier->send_notification(event_notifier, NULL, NULL, ¬if_ctx); return 0; } diff --git a/src/probes/lttng-uprobes.c b/src/probes/lttng-uprobes.c index 031bfa7c..bab941ab 100644 --- a/src/probes/lttng-uprobes.c +++ b/src/probes/lttng-uprobes.c @@ -69,11 +69,13 @@ int lttng_uprobes_event_notifier_handler_pre(struct uprobe_consumer *uc, struct struct lttng_uprobe_handler *uprobe_handler = container_of(uc, struct lttng_uprobe_handler, up_consumer); struct lttng_event_notifier *event_notifier = uprobe_handler->u.event_notifier; + struct lttng_kernel_notifier_ctx notif_ctx; if (unlikely(!READ_ONCE(event_notifier->enabled))) return 0; - event_notifier->send_notification(event_notifier, NULL, NULL); + notif_ctx.eval_capture = LTTNG_READ_ONCE(event_notifier->eval_capture); + event_notifier->send_notification(event_notifier, NULL, NULL, ¬if_ctx); return 0; } -- 2.34.1