function_graph: Add separate depth counter to prevent trace corruption
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest18 patches 24 repliesIn review
Patches in v1 No applies recorded yet
Reviews trailers per patch
3 of 18 patches have a Reviewed-by or Acked-by of their own; the series has one for all of them.
| Patch | Martin Schwidefsky | Masami Hiramatsu | Palmer Dabbelt | Will Deacon | Joe Lawrence |
|---|---|---|---|---|---|
| Series | R-b | T-b | |||
| 1/18 | |||||
| 2/18 | |||||
| 3/18 | |||||
| 4/18 | A-b | ||||
| 5/18 | |||||
| 6/18 | |||||
| 7/18 | |||||
| 8/18 | |||||
| 9/18 | |||||
| 10/18 | R-b | ||||
| 11/18 | A-b | ||||
| 12/18 | |||||
| 13/18 | |||||
| 14/18 | |||||
| 15/18 | |||||
| 16/18 | |||||
| 17/18 | |||||
| 18/18 |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes. Scroll the table sideways for all 5 reviewers.
Cover letter [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption
While working on rewriting function graph tracer, I found a design flaw in the code. Commit 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback") tried to fix a bug that caused interrupts not to be traced if max_depth was set to 1 (for debugging NO_HZ_FULL), because if it came in just as another function that was being traced that had a depth of 1 was exiting. This caused the timing of being in the kernel to be off. The fix was simply to move the calling of the return function after the updating of curr_ret_stack index as that was what was being used.
The change log even says that it was safe to do this, but unfortunately it was not, and the barrier() there was specifically *for* that function (which shows why one should document barriers).
The problem is that the return callback may still be using what's on the shadow stack and by changing the shadow stack pointer, it may allow for another function being traced to overwrite that data. Note, if this happens, it will only cause garbage data to be traced and will not affect the actual operations of the kernel (ie. it wont crash).
Unfortunately just reverting that will bring back the old bug. The real way to fix that old bug is to create another counter to handle the depth, but this means that we need to change all the architectures that implement function graph tracing (that's 12 of them). Luckily, I need to do this anyway in my re-design so this is a good thing.
Since all the archictectures do basicall the same thing to prepare the function graph trace to be traced, I made a generic function that they all can use and simplified the logic of all the architectures. Then I'm able to fix the design problem in one place.
I pushed this code up to let zero-day have a whack at it, and I also downloaded the latest 8.1.0 cross compilers for all the archs that are affected and compiled tested them all (and got rid of all the warnings I introduced as well).
I marked this all for stable, but in reality it may not need to be ported as it will probably not be trivial to do so, becaues you'll need to also fix the architectures that are no longer used (although do we care about them?). But if someone really cares about correct timings of the function graph profiler when the options/graph-time is set to zero, then be my guest.
Feel free to test this! I'll be pushing this to linux-next and let it sit there a week or so before pushing it to Linus.
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
ftrace/urgent
Head SHA1: 1ffd62e282a649483afb0bb4d76f7244b3c10075
Steven Rostedt (VMware) (18):
function_graph: Create function_graph_enter() to consolidate architecture code x86/function_graph: Simplify with function_graph_entry()
ARM: function_graph: Simplify with function_graph_entry()
arm64: function_graph: Simplify with function_graph_entry()
microblaze: function_graph: Simplify with function_graph_entry()
MIPS: function_graph: Simplify with function_graph_entry()
nds32: function_graph: Simplify with function_graph_entry()
parisc: function_graph: Simplify with function_graph_entry() powerpc/function_graph: Simplify with function_graph_entry() riscv/function_graph: Simplify with function_graph_entry() s390/function_graph: Simplify with function_graph_entry() sh/function_graph: Simplify with function_graph_entry() sparc/function_graph: Simplify with function_graph_entry()
function_graph: Make ftrace_push_return_trace() static
function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack
function_graph: Move return callback before update of curr_ret_stack
function_graph: Reverse the order of pushing the ret_stack and the callback
function_graph: Have profiler use curr_ret_stack and not depth
----
arch/arm/kernel/ftrace.c | 17 +------------ arch/arm64/kernel/ftrace.c | 15 +---------- arch/microblaze/kernel/ftrace.c | 15 ++--------- arch/mips/kernel/ftrace.c | 14 ++--------- arch/nds32/kernel/ftrace.c | 18 ++----------- arch/parisc/kernel/ftrace.c | 17 +++---------- arch/powerpc/kernel/trace/ftrace.c | 15 ++--------- arch/riscv/kernel/ftrace.c | 14 ++--------- arch/s390/kernel/ftrace.c | 13 ++-------- arch/sh/kernel/ftrace.c | 16 ++---------- arch/sparc/kernel/ftrace.c | 11 +------- arch/x86/kernel/ftrace.c | 15 +---------- include/linux/ftrace.h | 4 +-- include/linux/sched.h | 1 + kernel/trace/ftrace.c | 7 ++++-- kernel/trace/trace_functions_graph.c | 49 ++++++++++++++++++++++++++++-------- 16 files changed, 67 insertions(+), 174 deletions(-)