| Message ID | 56c113aa9c3e10c19144a36d9684c7882bf09af5.1606412433.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | powerpc64: Add support for ftrace direct calls | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (4c202167192a77481310a3cacae9f12618b92216) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 1 checks, 40 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On Thu, 26 Nov 2020 23:38:38 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > On powerpc, kprobe-direct.tc triggered FTRACE_WARN_ON() in > ftrace_get_addr_new() followed by the below message: > Bad trampoline accounting at: 000000004222522f (wake_up_process+0xc/0x20) (f0000001) > > The set of steps leading to this involved: > - modprobe ftrace-direct-too > - enable_probe > - modprobe ftrace-direct > - rmmod ftrace-direct <-- trigger > > The problem turned out to be that we were not updating flags in the > ftrace record properly. From the above message about the trampoline > accounting being bad, it can be seen that the ftrace record still has > FTRACE_FL_TRAMP set though ftrace-direct module is going away. This > happens because we are checking if any ftrace_ops has the > FTRACE_FL_TRAMP flag set _before_ updating the filter hash. > > The fix for this is to look for any _other_ ftrace_ops that also needs > FTRACE_FL_TRAMP. I'm applying this now and sending this for -rc and stable. The code worked on x86 because x86 has a way to make all users use trampolines, so this was never an issue (everything has a trampoline). I modified the kernel so that x86 would not create its own trampoline (see the weak function arch_ftrace_update_trampoline(), and I was able to reproduce the bug. I'm adding: Cc: stable@vger.kernel.org Fixes: a124692b698b0 ("ftrace: Enable trampoline when rec count returns back to one") Thanks! -- Steve > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > kernel/trace/ftrace.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 8185f7240095f4..9c1bba8cc51b03 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -1629,6 +1629,8 @@ static bool test_rec_ops_needs_regs(struct dyn_ftrace *rec) > static struct ftrace_ops * > ftrace_find_tramp_ops_any(struct dyn_ftrace *rec); > static struct ftrace_ops * > +ftrace_find_tramp_ops_any_other(struct dyn_ftrace *rec, struct ftrace_ops *op_exclude); > +static struct ftrace_ops * > ftrace_find_tramp_ops_next(struct dyn_ftrace *rec, struct ftrace_ops *ops); > > static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, > @@ -1778,7 +1780,7 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, > * to it. > */ > if (ftrace_rec_count(rec) == 1 && > - ftrace_find_tramp_ops_any(rec)) > + ftrace_find_tramp_ops_any_other(rec, ops)) > rec->flags |= FTRACE_FL_TRAMP; > else > rec->flags &= ~FTRACE_FL_TRAMP; > @@ -2244,6 +2246,24 @@ ftrace_find_tramp_ops_any(struct dyn_ftrace *rec) > return NULL; > } > > +static struct ftrace_ops * > +ftrace_find_tramp_ops_any_other(struct dyn_ftrace *rec, struct ftrace_ops *op_exclude) > +{ > + struct ftrace_ops *op; > + unsigned long ip = rec->ip; > + > + do_for_each_ftrace_op(op, ftrace_ops_list) { > + > + if (op == op_exclude || !op->trampoline) > + continue; > + > + if (hash_contains_ip(ip, op->func_hash)) > + return op; > + } while_for_each_ftrace_op(op); > + > + return NULL; > +} > + > static struct ftrace_ops * > ftrace_find_tramp_ops_next(struct dyn_ftrace *rec, > struct ftrace_ops *op)
Steven Rostedt wrote: > On Thu, 26 Nov 2020 23:38:38 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> On powerpc, kprobe-direct.tc triggered FTRACE_WARN_ON() in >> ftrace_get_addr_new() followed by the below message: >> Bad trampoline accounting at: 000000004222522f (wake_up_process+0xc/0x20) (f0000001) >> >> The set of steps leading to this involved: >> - modprobe ftrace-direct-too >> - enable_probe >> - modprobe ftrace-direct >> - rmmod ftrace-direct <-- trigger >> >> The problem turned out to be that we were not updating flags in the >> ftrace record properly. From the above message about the trampoline >> accounting being bad, it can be seen that the ftrace record still has >> FTRACE_FL_TRAMP set though ftrace-direct module is going away. This >> happens because we are checking if any ftrace_ops has the >> FTRACE_FL_TRAMP flag set _before_ updating the filter hash. >> >> The fix for this is to look for any _other_ ftrace_ops that also needs >> FTRACE_FL_TRAMP. > > I'm applying this now and sending this for -rc and stable. > > The code worked on x86 because x86 has a way to make all users use > trampolines, so this was never an issue (everything has a trampoline). > I modified the kernel so that x86 would not create its own trampoline > (see the weak function arch_ftrace_update_trampoline(), and I was able > to reproduce the bug. Good to know that you were able to reproduce this. > > I'm adding: > > Cc: stable@vger.kernel.org > Fixes: a124692b698b0 ("ftrace: Enable trampoline when rec count returns back to one") That looks good to me. Thanks for picking the two patches and for your review on the others! - Naveen
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 8185f7240095f4..9c1bba8cc51b03 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -1629,6 +1629,8 @@ static bool test_rec_ops_needs_regs(struct dyn_ftrace *rec) static struct ftrace_ops * ftrace_find_tramp_ops_any(struct dyn_ftrace *rec); static struct ftrace_ops * +ftrace_find_tramp_ops_any_other(struct dyn_ftrace *rec, struct ftrace_ops *op_exclude); +static struct ftrace_ops * ftrace_find_tramp_ops_next(struct dyn_ftrace *rec, struct ftrace_ops *ops); static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, @@ -1778,7 +1780,7 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, * to it. */ if (ftrace_rec_count(rec) == 1 && - ftrace_find_tramp_ops_any(rec)) + ftrace_find_tramp_ops_any_other(rec, ops)) rec->flags |= FTRACE_FL_TRAMP; else rec->flags &= ~FTRACE_FL_TRAMP; @@ -2244,6 +2246,24 @@ ftrace_find_tramp_ops_any(struct dyn_ftrace *rec) return NULL; } +static struct ftrace_ops * +ftrace_find_tramp_ops_any_other(struct dyn_ftrace *rec, struct ftrace_ops *op_exclude) +{ + struct ftrace_ops *op; + unsigned long ip = rec->ip; + + do_for_each_ftrace_op(op, ftrace_ops_list) { + + if (op == op_exclude || !op->trampoline) + continue; + + if (hash_contains_ip(ip, op->func_hash)) + return op; + } while_for_each_ftrace_op(op); + + return NULL; +} + static struct ftrace_ops * ftrace_find_tramp_ops_next(struct dyn_ftrace *rec, struct ftrace_ops *op)
On powerpc, kprobe-direct.tc triggered FTRACE_WARN_ON() in ftrace_get_addr_new() followed by the below message: Bad trampoline accounting at: 000000004222522f (wake_up_process+0xc/0x20) (f0000001) The set of steps leading to this involved: - modprobe ftrace-direct-too - enable_probe - modprobe ftrace-direct - rmmod ftrace-direct <-- trigger The problem turned out to be that we were not updating flags in the ftrace record properly. From the above message about the trampoline accounting being bad, it can be seen that the ftrace record still has FTRACE_FL_TRAMP set though ftrace-direct module is going away. This happens because we are checking if any ftrace_ops has the FTRACE_FL_TRAMP flag set _before_ updating the filter hash. The fix for this is to look for any _other_ ftrace_ops that also needs FTRACE_FL_TRAMP. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- kernel/trace/ftrace.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)