[PATCH] tracing: remove tracing_is_on export

[PATCH] tracing: remove tracing_is_on export

[Greg Kroah-Hartman]

The function tracing_is_on() is only called by in-kernel code, not by
any modules, so no need to export it as a symbol at all.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 kernel/trace/trace.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 578a49ff5c32..d09f2effa7a9 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1612,7 +1612,6 @@ int tracing_is_on(void)
 {
 	return tracer_tracing_is_on(&global_trace);
 }
-EXPORT_SYMBOL_GPL(tracing_is_on);
 
 static int __init set_buf_size(char *str)
 {
-- 
2.45.2

Re: [PATCH] tracing: remove tracing_is_on export

[Steven Rostedt]

On Thu, 25 Jul 2024 11:36:08 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> The function tracing_is_on() is only called by in-kernel code, not by
> any modules, so no need to export it as a symbol at all.

Hmm, this is part of the debugging code along with:

 tracing_on();  tracing_off();

I had it exported in case a module needed to use it in debugging.

-- Steve


> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  kernel/trace/trace.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 578a49ff5c32..d09f2effa7a9 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1612,7 +1612,6 @@ int tracing_is_on(void)
>  {
>  	return tracer_tracing_is_on(&global_trace);
>  }
> -EXPORT_SYMBOL_GPL(tracing_is_on);
>  
>  static int __init set_buf_size(char *str)
>  {

Re: [PATCH] tracing: remove tracing_is_on export

[Greg Kroah-Hartman]

On Thu, Jul 25, 2024 at 08:31:02AM -0400, Steven Rostedt wrote:
> On Thu, 25 Jul 2024 11:36:08 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > The function tracing_is_on() is only called by in-kernel code, not by
> > any modules, so no need to export it as a symbol at all.
> 
> Hmm, this is part of the debugging code along with:
> 
>  tracing_on();  tracing_off();
> 
> I had it exported in case a module needed to use it in debugging.

What module?  There is no in-kernel user of it as a module that I could
find, what am I missing?

thanks,

greg k-h

Re: [PATCH] tracing: remove tracing_is_on export

[Steven Rostedt]

On Thu, 25 Jul 2024 14:52:24 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jul 25, 2024 at 08:31:02AM -0400, Steven Rostedt wrote:
> > On Thu, 25 Jul 2024 11:36:08 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >   
> > > The function tracing_is_on() is only called by in-kernel code, not by
> > > any modules, so no need to export it as a symbol at all.  
> > 
> > Hmm, this is part of the debugging code along with:
> > 
> >  tracing_on();  tracing_off();
> > 
> > I had it exported in case a module needed to use it in debugging.  
> 
> What module?  There is no in-kernel user of it as a module that I could
> find, what am I missing?
>

Any module ;-)

It's for debugging. Just like trace_printk(). Something you would add to
debug a module and then delete it before submitting. It's why I put the
prototype into kernel.h. It's one of functions that can be handy during
development. It's not supposed to be submitted into the kernel.

Granted, tracing_is_on() is probably the least likely one to be used, but I
added it with the package, and I have actually used it for debugging a few
times.

-- Steve

Re: [PATCH] tracing: remove tracing_is_on export

[Greg Kroah-Hartman]

On Thu, Jul 25, 2024 at 09:26:09AM -0400, Steven Rostedt wrote:
> On Thu, 25 Jul 2024 14:52:24 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Jul 25, 2024 at 08:31:02AM -0400, Steven Rostedt wrote:
> > > On Thu, 25 Jul 2024 11:36:08 +0200
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >   
> > > > The function tracing_is_on() is only called by in-kernel code, not by
> > > > any modules, so no need to export it as a symbol at all.  
> > > 
> > > Hmm, this is part of the debugging code along with:
> > > 
> > >  tracing_on();  tracing_off();
> > > 
> > > I had it exported in case a module needed to use it in debugging.  
> > 
> > What module?  There is no in-kernel user of it as a module that I could
> > find, what am I missing?
> >
> 
> Any module ;-)
> 
> It's for debugging. Just like trace_printk(). Something you would add to
> debug a module and then delete it before submitting. It's why I put the
> prototype into kernel.h. It's one of functions that can be handy during
> development. It's not supposed to be submitted into the kernel.
> 
> Granted, tracing_is_on() is probably the least likely one to be used, but I
> added it with the package, and I have actually used it for debugging a few
> times.

Generally, we don't allow symbols that are not actually being used in
the kernel tree?  tracing_is_on() is a "code flow" type of thing, where
code can operate differently if it is enabled or not.

And I would argue that tracing_on() and tracing_off() should also not be
allowed to be in a module, why would you want that?  Just enable/disable
it from userspace when doing your testing, IF you have permission to do
so.

thanks,

greg k-h

Re: [PATCH] tracing: remove tracing_is_on export

[Steven Rostedt]

On Thu, 25 Jul 2024 15:35:00 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> Generally, we don't allow symbols that are not actually being used in
> the kernel tree?  tracing_is_on() is a "code flow" type of thing, where
> code can operate differently if it is enabled or not.
> 
> And I would argue that tracing_on() and tracing_off() should also not be
> allowed to be in a module, why would you want that?  Just enable/disable
> it from userspace when doing your testing, IF you have permission to do
> so.

tracing_off() is key to development, and one that I would argue very much
against removing. The other two are more just for "completeness".

It usually is used by adding a bunch of trace_printk(), and then:

	if (condition) {
		trace_printk("Condition hit!\n");
		tracing_off();
	}

And then you run your kernel until you noticed that something weird
happened. Then look at the trace file, and it will have all the events that
happened up to the anomaly condition, and you don't have to worry about the
ring buffer overflowing and losing your data.

This workflow is used by many developers.

-- Steve

Re: [PATCH] tracing: remove tracing_is_on export

[Greg Kroah-Hartman]

On Thu, Jul 25, 2024 at 09:53:07AM -0400, Steven Rostedt wrote:
> On Thu, 25 Jul 2024 15:35:00 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > Generally, we don't allow symbols that are not actually being used in
> > the kernel tree?  tracing_is_on() is a "code flow" type of thing, where
> > code can operate differently if it is enabled or not.
> > 
> > And I would argue that tracing_on() and tracing_off() should also not be
> > allowed to be in a module, why would you want that?  Just enable/disable
> > it from userspace when doing your testing, IF you have permission to do
> > so.
> 
> tracing_off() is key to development, and one that I would argue very much
> against removing. The other two are more just for "completeness".
> 
> It usually is used by adding a bunch of trace_printk(), and then:
> 
> 	if (condition) {
> 		trace_printk("Condition hit!\n");
> 		tracing_off();
> 	}
> 
> And then you run your kernel until you noticed that something weird
> happened. Then look at the trace file, and it will have all the events that
> happened up to the anomaly condition, and you don't have to worry about the
> ring buffer overflowing and losing your data.
> 
> This workflow is used by many developers.

Is it documented anywhere?  I've never heard of it before, and nothing
really describes this in Documentation/ that I can find.

But as you only want these to be exported to developer kernels, why not
say that and put that behind a debugging Kconfig option or something?
That way "vendor kernels" can properly disable this as they don't want
to give this type of functionality to random 3rd-party kernel modules.

thanks,

greg k-h

Re: [PATCH] tracing: remove tracing_is_on export

[Steven Rostedt]

On Thu, 25 Jul 2024 16:41:11 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

 This workflow is used by many developers.  
> 
> Is it documented anywhere?  I've never heard of it before, and nothing
> really describes this in Documentation/ that I can find.

It is mentioned, but I could expand on it more:

Documentation/trace/ftrace.rst:

  tracing_on:

        This sets or displays whether writing to the trace
        ring buffer is enabled. Echo 0 into this file to disable
        the tracer or 1 to enable it. Note, this only disables
        writing to the ring buffer, the tracing overhead may
        still be occurring.

        The kernel function tracing_off() can be used within the
        kernel to disable writing to the ring buffer, which will
        set this file to "0". User space can re-enable tracing by
        echoing "1" into the file.

> 
> But as you only want these to be exported to developer kernels, why not
> say that and put that behind a debugging Kconfig option or something?

Why add the burden of having to compile the core kernel to enable it? I use
this all the time.

> That way "vendor kernels" can properly disable this as they don't want
> to give this type of functionality to random 3rd-party kernel modules.

This has been exported since 2008. Has it ever been a problem in the last
16 years?

-- Steve

Re: [PATCH] tracing: remove tracing_is_on export

[Greg Kroah-Hartman]

On Thu, Jul 25, 2024 at 12:17:45PM -0400, Steven Rostedt wrote:
> On Thu, 25 Jul 2024 16:41:11 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
>  This workflow is used by many developers.  
> > 
> > Is it documented anywhere?  I've never heard of it before, and nothing
> > really describes this in Documentation/ that I can find.
> 
> It is mentioned, but I could expand on it more:
> 
> Documentation/trace/ftrace.rst:
> 
>   tracing_on:
> 
>         This sets or displays whether writing to the trace
>         ring buffer is enabled. Echo 0 into this file to disable
>         the tracer or 1 to enable it. Note, this only disables
>         writing to the ring buffer, the tracing overhead may
>         still be occurring.
> 
>         The kernel function tracing_off() can be used within the
>         kernel to disable writing to the ring buffer, which will
>         set this file to "0". User space can re-enable tracing by
>         echoing "1" into the file.

Seems "dangerous" that any random driver can control all of the tracing
system like this, but you do you :)

> > But as you only want these to be exported to developer kernels, why not
> > say that and put that behind a debugging Kconfig option or something?
> 
> Why add the burden of having to compile the core kernel to enable it? I use
> this all the time.
> 
> > That way "vendor kernels" can properly disable this as they don't want
> > to give this type of functionality to random 3rd-party kernel modules.
> 
> This has been exported since 2008. Has it ever been a problem in the last
> 16 years?

As I am finding out, yes, external modules are "abusing" this to do
different types of logic depending on if tracing is enabled or not for
various unknown reasons.  As there was no in-kernel user of this symbol,
I assumed it was just an oversight and should be removed.

I'll go ask the distro involved to just remove the symbol from their
kernels instead, but that feels like the wrong thing to do to me.

thanks,

greg k-h

Re: [PATCH] tracing: remove tracing_is_on export

[Steven Rostedt]

On Fri, 26 Jul 2024 10:15:14 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> > This has been exported since 2008. Has it ever been a problem in the last
> > 16 years?  
> 
> As I am finding out, yes, external modules are "abusing" this to do
> different types of logic depending on if tracing is enabled or not for
> various unknown reasons.  As there was no in-kernel user of this symbol,
> I assumed it was just an oversight and should be removed.
> 
> I'll go ask the distro involved to just remove the symbol from their
> kernels instead, but that feels like the wrong thing to do to me.

Interesting as I was unaware of this. I'm not against removing the
"is_on" from being exported, as that really was only there to be
consistent with the others.

-- Steve

Re: [PATCH] tracing: remove tracing_is_on export

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>