Re: [PATCH] tracepoints: Update static_call before tp_funcs when adding a tracepoint

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Jul 22, 2021, at 10:33 PM, rostedt rostedt@goodmis.org wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Because of the significant overhead that retpolines pose on indirect
> calls, the tracepoint code was updated to use the new "static_calls" that
> can modify the running code to directly call a function instead of using
> an indirect caller, and this function can be changed at runtime.
> 
> In the tracepoint code that calls all the registered callbacks that are
> attached to a tracepoint, the following is done:
> 
>	it_func_ptr = rcu_dereference_raw((&__tracepoint_##name)->funcs);
>	if (it_func_ptr) {
>		__data = (it_func_ptr)->data;
>		static_call(tp_func_##name)(__data, args);
>	}
> 
> If there's just a single callback, the static_call is updated to just call
> that callback directly. Once another handler is added, then the static
> caller is updated to call the iterator, that simply loops over all the
> funcs in the array and calls each of the callbacks like the old method
> using indirect calling.
> 
> The issue was discovered with a race between updating the funcs array and
> updating the static_call. The funcs array was updated first and then the
> static_call was updated. This is not an issue as long as the first element
> in the old array is the same as the first element in the new array. But
> that assumption is incorrect, because callbacks also have a priority
> field, and if there's a callback added that has a higher priority than the
> callback on the old array, then it will become the first callback in the
> new array. This means that it is possible to call the old callback with
> the new callback data element, which can cause a kernel panic.
> 

[...]

Looking into the various transitions, I suspect the issue runs much deeper than
this.

The sequence of transitions (number of probes) I'm considering is:

0->1
1->2
2->1
1->0
0->1
1->2

I come to three conclusions:

Where we have:

tracepoint_remove_func()

                tracepoint_update_call(tp, tp_funcs,
                                       tp_funcs[0].func != old[0].func);

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.

My second conclusion is that it's odd that transition 1->0 leaves the
prior function call in place even after it's been removed. When we go
back to 0->1, that function call may still be called even though the
function is not there anymore. And there is no RCU synchronization on
these transitions, so those are all possible scenarios.

My third conclusion is that we'd need synchronize RCU whenever tp_funcs[0].data
changes for transitions 1->2, 2->1, and 1->2 because the priorities don't guarantee
that the first callback stays in the first position, and we also need to rcu sync
unconditionally on transition 1->0. We currently only have sync RCU on transition
from 2->1 when tp_funcs[0].func changes, which is bogus in many ways.

Basically, transitions from the iterator to a specific function should be handled
with care (making sure the tp_funcs array is updated and rcu-sync is done), except
in the specific case where the prior tp->funcs was NULL, which skips the function
call. And unless there is a rcu-sync between the state transitions, we need to consider
all prior states as additional original state as well. Therefore, in a 1->0->1
transition sequence, it's very much possible that the old function ends up observing
the new callback's data unless we add some rcu sync in between.

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com