[PATCH] tracing: Decrement trace_array when bootconfig creates an instance

[PATCH] tracing: Decrement trace_array when bootconfig creates an instance

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The trace_array_get_by_name() creates a ftrace instance and
trace_array_put() is used to remove the reference. Even though the
trace_array_get_by_name() creates the instance, it also adds a reference
count to it, that prevents user space from removing it.

As the bootconfig just creates the instance on boot up, it should still be
used where it can be deleted by user space after boot. A trace_array_put()
is required to let that happen. Otherwise, the created instance can not
be removed.

Also, change the documentation on trace_array_get_by_name() to make this not
be so confusing.

Fixes: 4f712a4d04a4e ("tracing/boot: Add instance node support")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace.c      | 4 ++++
 kernel/trace/trace_boot.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6fed9b0a8d58..0a5569b1cace 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8602,6 +8602,10 @@ static int instance_mkdir(const char *name)
  * NOTE: This function increments the reference counter associated with the
  * trace array returned. This makes sure it cannot be freed while in use.
  * Use trace_array_put() once the trace array is no longer needed.
+ * If the trace_array is to be freed, trace_array_destroy() needs to
+ * be called after the trace_array_put(), or simply let user space delete
+ * it from the tracefs instances directory. But until the
+ * trace_array_put() is called, user space can not delete it.
  *
  */
 struct trace_array *trace_array_get_by_name(const char *name)
diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index cd541ac1cbc1..2f616cd926b0 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -327,6 +327,7 @@ trace_boot_init_instances(struct xbc_node *node)
 			continue;
 		}
 		trace_boot_init_one_instance(tr, inode);
+		trace_array_put(tr);
 	}
 }
 
-- 
2.20.1

Re: [PATCH] tracing: Decrement trace_array when bootconfig creates an instance

[Masami Hiramatsu]

On Fri, 24 Jan 2020 20:59:27 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> The trace_array_get_by_name() creates a ftrace instance and
> trace_array_put() is used to remove the reference. Even though the
> trace_array_get_by_name() creates the instance, it also adds a reference
> count to it, that prevents user space from removing it.
> 
> As the bootconfig just creates the instance on boot up, it should still be
> used where it can be deleted by user space after boot. A trace_array_put()
> is required to let that happen. Otherwise, the created instance can not
> be removed.
> 
> Also, change the documentation on trace_array_get_by_name() to make this not
> be so confusing.

Good catch! I misunderstood trace_array_put() would remove the instance.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> 
> Fixes: 4f712a4d04a4e ("tracing/boot: Add instance node support")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace.c      | 4 ++++
>  kernel/trace/trace_boot.c | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6fed9b0a8d58..0a5569b1cace 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -8602,6 +8602,10 @@ static int instance_mkdir(const char *name)
>   * NOTE: This function increments the reference counter associated with the
>   * trace array returned. This makes sure it cannot be freed while in use.
>   * Use trace_array_put() once the trace array is no longer needed.
> + * If the trace_array is to be freed, trace_array_destroy() needs to
> + * be called after the trace_array_put(), or simply let user space delete
> + * it from the tracefs instances directory. But until the
> + * trace_array_put() is called, user space can not delete it.
>   *
>   */
>  struct trace_array *trace_array_get_by_name(const char *name)
> diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> index cd541ac1cbc1..2f616cd926b0 100644
> --- a/kernel/trace/trace_boot.c
> +++ b/kernel/trace/trace_boot.c
> @@ -327,6 +327,7 @@ trace_boot_init_instances(struct xbc_node *node)
>  			continue;
>  		}
>  		trace_boot_init_one_instance(tr, inode);
> +		trace_array_put(tr);
>  	}
>  }
>  
> -- 
> 2.20.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] tracing: Decrement trace_array when bootconfig creates an instance

[Steven Rostedt]

On Sat, 25 Jan 2020 11:23:00 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Fri, 24 Jan 2020 20:59:27 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > The trace_array_get_by_name() creates a ftrace instance and
> > trace_array_put() is used to remove the reference. Even though the
> > trace_array_get_by_name() creates the instance, it also adds a reference
> > count to it, that prevents user space from removing it.
> > 
> > As the bootconfig just creates the instance on boot up, it should still be
> > used where it can be deleted by user space after boot. A trace_array_put()
> > is required to let that happen. Otherwise, the created instance can not
> > be removed.
> > 
> > Also, change the documentation on trace_array_get_by_name() to make this not
> > be so confusing.  
> 
> Good catch! I misunderstood trace_array_put() would remove the instance.

I figured as much, which is why I included the documentation update.

> 
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

-- Steve