| Message ID | 5962b9e161b2b9296509c3d65c7f16c11b07bd16.1727904678.git.jpoimboe@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | parisc/ftrace: Fix function graph tracing disablement | expand |
On Wed, 2 Oct 2024 14:32:04 -0700 Josh Poimboeuf <jpoimboe@kernel.org> wrote: > Due to an apparent copy-paste bug, the parisc implementation of > ftrace_disable_ftrace_graph_caller() doesn't actually do anything. It > enables the (already-enabled) static key rather than disabling it. > > The result is that after function graph tracing has been "disabled", any > subsequent (non-graph) function tracing will inadvertently also enable > the slow fgraph return address hijacking. > > Fixes: 98f2926171ae ("parisc/ftrace: use static key to enable/disable function graph tracer") > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > arch/parisc/kernel/ftrace.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c > index c91f9c2e61ed..f8d08eab7db8 100644 > --- a/arch/parisc/kernel/ftrace.c > +++ b/arch/parisc/kernel/ftrace.c > @@ -87,7 +87,7 @@ int ftrace_enable_ftrace_graph_caller(void) > > int ftrace_disable_ftrace_graph_caller(void) > { > - static_key_enable(&ftrace_graph_enable.key); > + static_key_disable(&ftrace_graph_enable.key); > return 0; > } > #endif Awfully quite :-/ I wonder if anyone cares about parisc today? Hmm. -- Steve
On Thu, Oct 10, 2024 at 03:48:40PM -0400, Steven Rostedt wrote: > > +++ b/arch/parisc/kernel/ftrace.c > > @@ -87,7 +87,7 @@ int ftrace_enable_ftrace_graph_caller(void) > > > > int ftrace_disable_ftrace_graph_caller(void) > > { > > - static_key_enable(&ftrace_graph_enable.key); > > + static_key_disable(&ftrace_graph_enable.key); > > return 0; > > } > > #endif > > Awfully quite :-/ > > I wonder if anyone cares about parisc today? > > Hmm. I still see some git activity in arch/parisc, so apparently so... I only stumbled on this by accident (code inspection) while making another unrelated change.
On 10/10/24 21:56, Josh Poimboeuf wrote: > On Thu, Oct 10, 2024 at 03:48:40PM -0400, Steven Rostedt wrote: >>> +++ b/arch/parisc/kernel/ftrace.c >>> @@ -87,7 +87,7 @@ int ftrace_enable_ftrace_graph_caller(void) >>> >>> int ftrace_disable_ftrace_graph_caller(void) >>> { >>> - static_key_enable(&ftrace_graph_enable.key); >>> + static_key_disable(&ftrace_graph_enable.key); >>> return 0; >>> } >>> #endif >> >> Awfully quite :-/ >> >> I wonder if anyone cares about parisc today? Actually I do, and yes we have users. Of course in reality that's "fun" users and not "productive" usage. From programming perspective parisc is quite interesting since it has an upwards-growing stack, limited ways of atomic ops, and some awkward cache dependencies, so I regularly find issues in generic Linux code which would not be found otherwise. > I still see some git activity in arch/parisc, so apparently so... > > I only stumbled on this by accident (code inspection) while making > another unrelated change. Thanks for your patch! I'll apply it in my for-next git tree and push upstream at some point (unless someone else takes it before me). Helge
On Thu, 10 Oct 2024 22:25:13 +0200 Helge Deller <deller@gmx.de> wrote: > > Actually I do, and yes we have users. > Of course in reality that's "fun" users and not "productive" usage. We welcome those "fun" users as well! > > From programming perspective parisc is quite interesting since it > has an upwards-growing stack, limited ways of atomic ops, > and some awkward cache dependencies, so I regularly find > issues in generic Linux code which would not be found otherwise. > > > I still see some git activity in arch/parisc, so apparently so... > > > > I only stumbled on this by accident (code inspection) while making > > another unrelated change. > > Thanks for your patch! > I'll apply it in my for-next git tree and push upstream at some point > (unless someone else takes it before me). For architecture patches like this, even if it is obvious, I rather it go through them, as I wouldn't even compile test it. Please take it through your tree. Thanks, -- Steve
diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c index c91f9c2e61ed..f8d08eab7db8 100644 --- a/arch/parisc/kernel/ftrace.c +++ b/arch/parisc/kernel/ftrace.c @@ -87,7 +87,7 @@ int ftrace_enable_ftrace_graph_caller(void) int ftrace_disable_ftrace_graph_caller(void) { - static_key_enable(&ftrace_graph_enable.key); + static_key_disable(&ftrace_graph_enable.key); return 0; } #endif
Due to an apparent copy-paste bug, the parisc implementation of ftrace_disable_ftrace_graph_caller() doesn't actually do anything. It enables the (already-enabled) static key rather than disabling it. The result is that after function graph tracing has been "disabled", any subsequent (non-graph) function tracing will inadvertently also enable the slow fgraph return address hijacking. Fixes: 98f2926171ae ("parisc/ftrace: use static key to enable/disable function graph tracer") Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- arch/parisc/kernel/ftrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)