On Thu, Oct 01, 2020 at 09:27:57PM -0400, Steven Rostedt wrote: > From: Steven Rostedt (VMware) <[email protected]> > > Naresh reported a bug discovered in linux-next that I can reliably > trigger myself. It appears to be a side effect of the static calls. It > happens when going from more than one tracepoint callback to a single > one, and removing the first callback on the list. The list of > tracepoint callbacks holds data and a function to call with the > parameters of that tracepoint and a handler to the associated data. > > old_list: > 0: func = foo; data = NULL; > 1: func = bar; data = &bar_struct; > > new_list: > 0: func = bar; data = &bar_struct; > > > CPU 0 CPU 1 > ----- ----- > tp_funcs = old_list; > tp_static_caller = tp_interator > > __DO_TRACE() > > data = tp_funcs[0].data = NULL; > > tp_funcs = new_list; > tracepoint_update_call() > tp_static_caller = tp_funcs[0] = bar; > tp_static_caller(data) > bar(data) > x = data->item = NULL->item > > BOOM!
> To solve this, add a tracepoint_synchronize_unregister() between > changing tp_funcs and updating the static tracepoint, that does both a > synchronize_rcu() and synchronize_srcu(). This will ensure that when > the static call is updated to the single callback that it will be > receiving the data that it registered with. > Reported-by: Naresh Kamboju <[email protected]> > Fixes: d25e37d89dd2f ("tracepoint: Optimize using static_call()") > Signed-off-by: Steven Rostedt (VMware) <[email protected]> > --- Urgh :/ I'll go stick this in tip/core/static_call. > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 1b4be44d1d2b..3f659f855074 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -221,7 +221,7 @@ static void *func_remove(struct tracepoint_func **funcs, > return old; > } > > -static void tracepoint_update_call(struct tracepoint *tp, struct > tracepoint_func *tp_funcs) > +static void tracepoint_update_call(struct tracepoint *tp, struct > tracepoint_func *tp_funcs, bool sync) > { > void *func = tp->iterator; > > @@ -229,8 +229,17 @@ static void tracepoint_update_call(struct tracepoint > *tp, struct tracepoint_func > if (!tp->static_call_key) > return; > > - if (!tp_funcs[1].func) > + if (!tp_funcs[1].func) { > 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); > } > @@ -265,7 +274,7 @@ static int tracepoint_add_func(struct tracepoint *tp, > * include/linux/tracepoint.h using rcu_dereference_sched(). > */ > rcu_assign_pointer(tp->funcs, tp_funcs); > - tracepoint_update_call(tp, tp_funcs); > + tracepoint_update_call(tp, tp_funcs, false); > static_key_enable(&tp->key); > > release_probes(old); > @@ -297,11 +306,12 @@ static int tracepoint_remove_func(struct tracepoint *tp, > tp->unregfunc(); > > static_key_disable(&tp->key); > + rcu_assign_pointer(tp->funcs, tp_funcs); > } else { > - tracepoint_update_call(tp, tp_funcs); > + rcu_assign_pointer(tp->funcs, tp_funcs); > + tracepoint_update_call(tp, tp_funcs, > + tp_funcs[0].func != old[0].func); > } > - > - rcu_assign_pointer(tp->funcs, tp_funcs); > release_probes(old); > return 0; > }
