tracing: Fix tracepoint and static call race condition`
Subsystems (2)
Steven Rostedt
maintainercover tracing: Fix tracepoint and static call race condition`
Fix tracepoint race between static_call and callback data
As callbacks to a tracepoint are paired with the data that is passed in when the callback is registered to the tracepoint, it must have that data passed to the callback when the tracepoint is triggered, else bad things will happen. To keep the two together, they are both assigned to a tracepoint structure and added to an array. The tracepoint call site will dereference the structure (via RCU) and call the callback in that structure along with the data in that structure. This keeps the callback and data tightly coupled.
Because of the overhead that retpolines have on tracepoint callbacks, if there's only one callback attached to a tracepoint (a common case), then it is called via a static call (code modified to do a direct call instead of an indirect call). But to implement this, the data had to be decoupled from the callback, as now the callback is implemented via a direct call from the static call and not an indirect call from the dereferenced structure.
Note, the static call only calls a callback used when there's a single callback attached to the tracepoint. If more than one callback is attached to the same tracepoint, then the static call will call an iterator function that goes back to dereferencing the structure keeping the callback and its data tightly coupled again.
Issues can arise when going from 0 callbacks to one, as the static call is assigned to the callback, and it must take care that the data passed to it is loaded before the static call calls the callback. Going from 1 to 2 callbacks is not an issue, as long as the static call is updated to the iterator before the tracepoint structure array is updated via RCU. Going from 2 to more or back down to 2 is not an issue as the iterator can handle all theses cases. But going from 2 to 1, care must be taken as the static call is now calling a callback and the data that is loaded must be the data for that callback.
Care was taken to ensure the callback and data would be in-sync, but after a bug was reported, it became clear that not enough was done to make sure that was the case. These changes address this.
The first change is to compare the old and new data instead of the old and new callback, as it's the data that can corrupt the callback, even if the callback is the same (something getting freed).
The next change is to convert these transitions into states, to make it easier to know when a synchronization is needed, and to perform those synchronizations. The problem with this patch is that it slows down disabling all events from under a second, to making it take over 10 seconds to do the same work. But that is addressed in the final patch.
The final patch uses the RCU state functions to keep track of the RCU state between the transitions, and only needs to perform the synchronization if an RCU synchronization hasn't been done already. This brings the performance of disabling all events back to its original value. That's because no synchronization is required between disabling tracepoints. It is only required when enabling a tracepoint after its been disabled. If an RCU synchronization happens after the tracepoint is disabled, and before it is re-enabled, there's no need to do the synchronization again.
Both the second and third patch have subtle complexities that they are separated into two patches. But because the second patch causes such a regression in performance, the third patch adds a "Fixes" tag to the second patch, such that the two must be backported together and not just the second patch.
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
for-next
Head SHA1: 79497d2cb5619c45a5e259d27280ead1060b776b
Mathieu Desnoyers (3):
tracepoint: static call: Compare data on transition from 2->1 callees
tracepoint: Fix static call function vs data state mismatch
tracepoint: Use rcu get state and cond sync for static call updates
----
kernel/tracepoint.c | 155 +++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 135 insertions(+), 20 deletions(-)
Steven Rostedt
maintainerPATCH 1/3 tracepoint: static call: Compare data on transition from 2->1 callees
From: Mathieu Desnoyers <[email protected]>
On transition from 2->1 callees, we should be comparing .data rather than .func, because the same callback can be registered twice with different data, and what we care about here is that the data of array element 0 is unchanged to skip rcu sync.
--- kernel/tracepoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/tracepoint.c +1 -1
@@ -338,7 +338,7 @@ static int tracepoint_remove_func(struct tracepoint *tp, }else {rcu_assign_pointer (tp->funcs, tp_funcs);tracepoint_update_call (tp, tp_funcs, - tp_funcs[0 ].func != old[0 ].func); + tp_funcs[0 ].data != old[0 ].data); }release_probes (old);return 0 ;
Signature · 1 line
2.30.2
Steven Rostedt
maintainerPATCH 2/3 tracepoint: Fix static call function vs data state mismatch
In reply to Steven RostedtFrom: Mathieu Desnoyers <[email protected]>
On a 1->0->1 callbacks transition, there is an issue with the new callback using the old callback's data.
Considering __DO_TRACE_CALL:
do { \
struct tracepoint_func *it_func_ptr; \
void *__data; \
it_func_ptr = \
rcu_dereference_raw((&__tracepoint_##name)->funcs); \
if (it_func_ptr) { \
__data = (it_func_ptr)->data; \----> [ delayed here on one CPU (e.g. vcpu preempted by the host) ]
static_call(tp_func_##name)(__data, args); \
} \
} while (0)It has loaded the tp->funcs of the old callback, so it will try to use the old data. This can be fixed by adding a RCU sync anywhere in the 1->0->1 transition chain.
On a N->2->1 transition, we need an rcu-sync because you may have a sequence of 3->2->1 (or 1->2->1) where the element 0 data is unchanged between 2->1, but was changed from 3->2 (or from 1->2), which may be observed by the static call. This can be fixed by adding an unconditional RCU sync in transition 2->1.
Note, this fixes a correctness issue at the cost of adding a tremendous performance regression to the disabling of tracepoints.
Before this commit:
# trace-cmd start -e all # time trace-cmd start -p nop
real 0m0.778s
user 0m0.000s
sys 0m0.061s
After this commit:
# trace-cmd start -e all # time trace-cmd start -p nop
real 0m10.593s
user 0m0.017s
sys 0m0.259s
A follow up fix will introduce a more lightweight scheme based on RCU get_state and cond_sync, that will return the performance back to what it was. As both this change and the lightweight versions are complex on their own, for bisecting any issues that this may cause, they are kept as two separate changes.
--- kernel/tracepoint.c | 102 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 20 deletions(-)
kernel/tracepoint.c +82 -20
@@ -15,6 +15,13 @@#include <linux/sched/task.h> #include <linux/static_key.h> +enum tp_func_state { +TP_FUNC_0 , +TP_FUNC_1 , +TP_FUNC_2 , +TP_FUNC_N , +}; +extern tracepoint_ptr_t __start___tracepoints_ptrs[];extern tracepoint_ptr_t __stop___tracepoints_ptrs[];
@@ -246,26 +253,29 @@ static void *func_remove(struct tracepoint_func **funcs,return old; } -static void tracepoint_update_call (struct tracepoint *tp,struct tracepoint_func *tp_funcs,bool sync) +/* +* Count the number of functions (enum tp_func_state) in a tp_funcs array. +*/ +static enum tp_func_state nr_func_state (const struct tracepoint_func *tp_funcs) +{ +if (!tp_funcs) +return TP_FUNC_0 ; +if (!tp_funcs[1 ].func) +return TP_FUNC_1 ; +if (!tp_funcs[2 ].func) +return TP_FUNC_2 ; +return TP_FUNC_N ;/* 3 or more */ +} + +static void tracepoint_update_call (struct tracepoint *tp,struct tracepoint_func *tp_funcs) {void *func = tp->iterator;/* Synthetic events do not have static call sites */ if (!tp->static_call_key)return ; - -if (!tp_funcs[1 ].func) { +if (nr_func_state (tp_funcs) ==TP_FUNC_1 ) func = tp_funcs[0 ].func; -/* -* If going from the iterator back to a single caller, -* we need to synchronize with __DO_TRACE to make sure -* that the data passed to the callback is the one that -* belongs to that callback. -*/ -if (sync) -tracepoint_synchronize_unregister (); - } -__static_call_update (tp->static_call_key, tp->static_call_tramp, func); }
@@ -299,9 +309,31 @@ static int tracepoint_add_func(struct tracepoint *tp,* a pointer to it. This array is referenced by __DO_TRACE from * include/linux/tracepoint.h using rcu_dereference_sched(). */ -tracepoint_update_call (tp, tp_funcs, false); -rcu_assign_pointer (tp->funcs, tp_funcs); -static_key_enable (&tp->key); +switch (nr_func_state (tp_funcs)) { +case TP_FUNC_1 :/* 0->1 */ +/* Set static call to first function */ +tracepoint_update_call (tp, tp_funcs); +/* Both iterator and static call handle NULL tp->funcs */ +rcu_assign_pointer (tp->funcs, tp_funcs); +static_key_enable (&tp->key); +break ; +case TP_FUNC_2 :/* 1->2 */ +/* Set iterator static call */ +tracepoint_update_call (tp, tp_funcs); +/* +* Iterator callback installed before updating tp->funcs. +* Requires ordering between RCU assign/dereference and +* static call update/call. +*/ +rcu_assign_pointer (tp->funcs, tp_funcs); +break ; +case TP_FUNC_N :/* N->N+1 (N>1) */ +rcu_assign_pointer (tp->funcs, tp_funcs); +break ; +default : +WARN_ON_ONCE (1 ); +break ; + }release_probes (old);return 0 ;
@@ -328,17 +360,47 @@ static int tracepoint_remove_func(struct tracepoint *tp,/* Failed allocating new tp_funcs, replaced func with stub */ return 0 ; -if (!tp_funcs) { +switch (nr_func_state (tp_funcs)) { +case TP_FUNC_0 :/* 1->0 */ /* Removed last function */ if (tp->unregfunc &&static_key_enabled (&tp->key)) tp->unregfunc ();static_key_disable (&tp->key); +/* Set iterator static call */ +tracepoint_update_call (tp, tp_funcs); +/* Both iterator and static call handle NULL tp->funcs */ +rcu_assign_pointer (tp->funcs,NULL ); +/* +* Make sure new func never uses old data after a 1->0->1 +* transition sequence. +* Considering that transition 0->1 is the common case +* and don't have rcu-sync, issue rcu-sync after +* transition 1->0 to break that sequence by waiting for +* readers to be quiescent. +*/ +tracepoint_synchronize_unregister (); +break ; +case TP_FUNC_1 :/* 2->1 */ rcu_assign_pointer (tp->funcs, tp_funcs); - }else { +/* +* On 2->1 transition, RCU sync is needed before setting +* static call to first callback, because the observer +* may have loaded any prior tp->funcs after the last one +* associated with an rcu-sync. +*/ +tracepoint_synchronize_unregister (); +/* Set static call to first function */ +tracepoint_update_call (tp, tp_funcs); +break ; +case TP_FUNC_2 :/* N->N-1 (N>2) */ + fallthrough; +case TP_FUNC_N :rcu_assign_pointer (tp->funcs, tp_funcs); -tracepoint_update_call (tp, tp_funcs, - tp_funcs[0 ].data != old[0 ].data); +break ; +default : +WARN_ON_ONCE (1 ); +break ; }release_probes (old);return 0 ;
Signature · 1 line
2.30.2
Steven Rostedt
maintainerPATCH 3/3 tracepoint: Use rcu get state and cond sync for static call updates
In reply to Steven RostedtFrom: Mathieu Desnoyers <[email protected]>
State transitions from 1->0->1 and N->2->1 callbacks require RCU synchronization. Rather than performing the RCU synchronization every time the state change occurs, which is quite slow when many tracepoints are registered in batch, instead keep a snapshot of the RCU state on the most recent transitions which belong to a chain, and conditionally wait for a grace period on the last transition of the chain if one g.p. has not elapsed since the last snapshot.
This applies to both RCU and SRCU.
This brings the performance regression caused by commit 231264d6927f ("Fix: tracepoint: static call function vs data state mismatch") back to what it was originally.
Before this commit:
# trace-cmd start -e all # time trace-cmd start -p nop
real 0m10.593s
user 0m0.017s
sys 0m0.259s
After this commit:
# trace-cmd start -e all # time trace-cmd start -p nop
real 0m0.878s
user 0m0.000s
sys 0m0.103s
--- kernel/tracepoint.c | 81 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 14 deletions(-)
kernel/tracepoint.c +67 -14
@@ -28,6 +28,44 @@ extern tracepoint_ptr_t __stop___tracepoints_ptrs[];DEFINE_SRCU (tracepoint_srcu);EXPORT_SYMBOL_GPL (tracepoint_srcu); +enum tp_transition_sync { +TP_TRANSITION_SYNC_1_0_1 , +TP_TRANSITION_SYNC_N_2_1 , + + _NR_TP_TRANSITION_SYNC, +}; + +struct tp_transition_snapshot { +unsigned long rcu; +unsigned long srcu; +bool ongoing; +}; + +/* Protected by tracepoints_mutex */ +static struct tp_transition_snapshot tp_transition_snapshot[_NR_TP_TRANSITION_SYNC]; + +static void tp_rcu_get_state (enum tp_transition_sync sync) +{ +struct tp_transition_snapshot *snapshot = &tp_transition_snapshot[sync]; + +/* Keep the latest get_state snapshot. */ + snapshot->rcu =get_state_synchronize_rcu (); + snapshot->srcu =start_poll_synchronize_srcu (&tracepoint_srcu); + snapshot->ongoing = true; +} + +static void tp_rcu_cond_sync (enum tp_transition_sync sync) +{ +struct tp_transition_snapshot *snapshot = &tp_transition_snapshot[sync]; + +if (!snapshot->ongoing) +return ; +cond_synchronize_rcu (snapshot->rcu); +if (!poll_state_synchronize_srcu (&tracepoint_srcu, snapshot->srcu)) +synchronize_srcu (&tracepoint_srcu); + snapshot->ongoing = false; +} +/* Set to 1 to enable tracepoint debug output */ static const int tracepoint_debug;
@@ -311,6 +349,11 @@ static int tracepoint_add_func(struct tracepoint *tp,*/ switch (nr_func_state (tp_funcs)) {case TP_FUNC_1 :/* 0->1 */ +/* +* Make sure new static func never uses old data after a +* 1->0->1 transition sequence. +*/ +tp_rcu_cond_sync (TP_TRANSITION_SYNC_1_0_1 );/* Set static call to first function */ tracepoint_update_call (tp, tp_funcs);/* Both iterator and static call handle NULL tp->funcs */
@@ -325,10 +368,15 @@ static int tracepoint_add_func(struct tracepoint *tp,* Requires ordering between RCU assign/dereference and * static call update/call. */ -rcu_assign_pointer (tp->funcs, tp_funcs); -break ; + fallthrough;case TP_FUNC_N :/* N->N+1 (N>1) */ rcu_assign_pointer (tp->funcs, tp_funcs); +/* +* Make sure static func never uses incorrect data after a +* N->...->2->1 (N>1) transition sequence. +*/ +if (tp_funcs[0 ].data != old[0 ].data) +tp_rcu_get_state (TP_TRANSITION_SYNC_N_2_1 );break ;default :WARN_ON_ONCE (1 );
@@ -372,24 +420,23 @@ static int tracepoint_remove_func(struct tracepoint *tp,/* Both iterator and static call handle NULL tp->funcs */ rcu_assign_pointer (tp->funcs,NULL );/* -* Make sure new func never uses old data after a 1->0->1 -* transition sequence. -* Considering that transition 0->1 is the common case -* and don't have rcu-sync, issue rcu-sync after -* transition 1->0 to break that sequence by waiting for -* readers to be quiescent. +* Make sure new static func never uses old data after a +* 1->0->1 transition sequence. */ -tracepoint_synchronize_unregister (); +tp_rcu_get_state (TP_TRANSITION_SYNC_1_0_1 );break ;case TP_FUNC_1 :/* 2->1 */ rcu_assign_pointer (tp->funcs, tp_funcs);/* -* On 2->1 transition, RCU sync is needed before setting -* static call to first callback, because the observer -* may have loaded any prior tp->funcs after the last one -* associated with an rcu-sync. +* Make sure static func never uses incorrect data after a +* N->...->2->1 (N>2) transition sequence. If the first +* element's data has changed, then force the synchronization +* to prevent current readers that have loaded the old data +* from calling the new function. */ -tracepoint_synchronize_unregister (); +if (tp_funcs[0 ].data != old[0 ].data) +tp_rcu_get_state (TP_TRANSITION_SYNC_N_2_1 ); +tp_rcu_cond_sync (TP_TRANSITION_SYNC_N_2_1 );/* Set static call to first function */ tracepoint_update_call (tp, tp_funcs);break ;
@@ -397,6 +444,12 @@ static int tracepoint_remove_func(struct tracepoint *tp, fallthrough;case TP_FUNC_N :rcu_assign_pointer (tp->funcs, tp_funcs); +/* +* Make sure static func never uses incorrect data after a +* N->...->2->1 (N>2) transition sequence. +*/ +if (tp_funcs[0 ].data != old[0 ].data) +tp_rcu_get_state (TP_TRANSITION_SYNC_N_2_1 );break ;default :WARN_ON_ONCE (1 );
Signature · 1 line
2.30.2