[PATCH 0/2] tracing: Clean up how iter is freed

[PATCH 0/2] tracing: Clean up how iter is freed

[Steven Rostedt]

The trace iterator is used in various interfaces and needs to be consistent
in how it is cleaned up. Add a helper function to clean up its content. But
before doing so, I noticed that iter->trace is allocated then the content
of tr->current_trace is copied to it. There's no reason for this, so the
first patch removes that allocation and just points to the content of
tr->current_trace, as tr->current_trace can change, but the content should
not.

Steven Rostedt (Google) (2):
      tracing: Remove unnecessary copying of tr->current_trace
      tracing: Add free_trace_iter_content() helper function
----
 kernel/trace/trace.c | 62 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

[PATCH 1/2] tracing: Remove unnecessary copying of tr->current_trace

[Steven Rostedt]

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

I have no idea why the iterator had to allocate a descriptor to make a
copy of "current_trace" (now "tr->current_trace"). The content of that
pointer never changes, so it's sufficient to just copy the pointer to
maintain integrity with reading it.

This is more of a clean up than a fix.

Fixes: d7350c3f45694 ("tracing/core: make the read callbacks reentrants")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index be847d45d81c..1c370ffbe062 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4205,15 +4205,9 @@ static void *s_start(struct seq_file *m, loff_t *pos)
 	loff_t l = 0;
 	int cpu;
 
-	/*
-	 * copy the tracer to avoid using a global lock all around.
-	 * iter->trace is a copy of current_trace, the pointer to the
-	 * name may be used instead of a strcmp(), as iter->trace->name
-	 * will point to the same string as current_trace->name.
-	 */
 	mutex_lock(&trace_types_lock);
-	if (unlikely(tr->current_trace && iter->trace->name != tr->current_trace->name))
-		*iter->trace = *tr->current_trace;
+	if (unlikely(tr->current_trace != iter->trace))
+		iter->trace = tr->current_trace;
 	mutex_unlock(&trace_types_lock);
 
 #ifdef CONFIG_TRACER_MAX_TRACE
@@ -4862,16 +4856,8 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 	iter->fmt = NULL;
 	iter->fmt_size = 0;
 
-	/*
-	 * We make a copy of the current tracer to avoid concurrent
-	 * changes on it while we are reading.
-	 */
 	mutex_lock(&trace_types_lock);
-	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
-	if (!iter->trace)
-		goto fail;
-
-	*iter->trace = *tr->current_trace;
+	iter->trace = tr->current_trace;
 
 	if (!zalloc_cpumask_var(&iter->started, GFP_KERNEL))
 		goto fail;
@@ -4936,7 +4922,6 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 
  fail:
 	mutex_unlock(&trace_types_lock);
-	kfree(iter->trace);
 	kfree(iter->temp);
 	kfree(iter->buffer_iter);
 release:
@@ -5021,7 +5006,6 @@ static int tracing_release(struct inode *inode, struct file *file)
 	free_cpumask_var(iter->started);
 	kfree(iter->fmt);
 	kfree(iter->temp);
-	kfree(iter->trace);
 	kfree(iter->buffer_iter);
 	seq_release_private(inode, file);
 
-- 
2.40.1

Re: [PATCH 1/2] tracing: Remove unnecessary copying of tr->current_trace

[Masami Hiramatsu]

On Thu, 13 Jul 2023 11:46:26 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> I have no idea why the iterator had to allocate a descriptor to make a
> copy of "current_trace" (now "tr->current_trace"). The content of that
> pointer never changes, so it's sufficient to just copy the pointer to
> maintain integrity with reading it.
> 
> This is more of a clean up than a fix.

Interesting bug. Anyway looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

> 
> Fixes: d7350c3f45694 ("tracing/core: make the read callbacks reentrants")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index be847d45d81c..1c370ffbe062 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4205,15 +4205,9 @@ static void *s_start(struct seq_file *m, loff_t *pos)
>  	loff_t l = 0;
>  	int cpu;
>  
> -	/*
> -	 * copy the tracer to avoid using a global lock all around.
> -	 * iter->trace is a copy of current_trace, the pointer to the
> -	 * name may be used instead of a strcmp(), as iter->trace->name
> -	 * will point to the same string as current_trace->name.
> -	 */
>  	mutex_lock(&trace_types_lock);
> -	if (unlikely(tr->current_trace && iter->trace->name != tr->current_trace->name))
> -		*iter->trace = *tr->current_trace;
> +	if (unlikely(tr->current_trace != iter->trace))
> +		iter->trace = tr->current_trace;
>  	mutex_unlock(&trace_types_lock);
>  
>  #ifdef CONFIG_TRACER_MAX_TRACE
> @@ -4862,16 +4856,8 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  	iter->fmt = NULL;
>  	iter->fmt_size = 0;
>  
> -	/*
> -	 * We make a copy of the current tracer to avoid concurrent
> -	 * changes on it while we are reading.
> -	 */
>  	mutex_lock(&trace_types_lock);
> -	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
> -	if (!iter->trace)
> -		goto fail;
> -
> -	*iter->trace = *tr->current_trace;
> +	iter->trace = tr->current_trace;
>  
>  	if (!zalloc_cpumask_var(&iter->started, GFP_KERNEL))
>  		goto fail;
> @@ -4936,7 +4922,6 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  
>   fail:
>  	mutex_unlock(&trace_types_lock);
> -	kfree(iter->trace);
>  	kfree(iter->temp);
>  	kfree(iter->buffer_iter);
>  release:
> @@ -5021,7 +5006,6 @@ static int tracing_release(struct inode *inode, struct file *file)
>  	free_cpumask_var(iter->started);
>  	kfree(iter->fmt);
>  	kfree(iter->temp);
> -	kfree(iter->trace);
>  	kfree(iter->buffer_iter);
>  	seq_release_private(inode, file);
>  
> -- 
> 2.40.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH 2/2] tracing: Add free_trace_iter_content() helper function

[Steven Rostedt]

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

As the trace iterator is created and used by various interfaces, the clean
up of it needs to be consistent. Create a free_trace_iter_content() helper
function that frees the content of the iterator and use that to clean it
up in all places that it is used.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 1c370ffbe062..3f38250637e2 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4815,6 +4815,25 @@ static const struct seq_operations tracer_seq_ops = {
 	.show		= s_show,
 };
 
+/*
+ * Note, as iter itself can be allocated and freed in different
+ * ways, this function is only used to free its content, and not
+ * the iterator itself. The only requirement to all the allocations
+ * is that it must zero all fields (kzalloc), as freeing works with
+ * ethier allocated content or NULL.
+ */
+static void free_trace_iter_content(struct trace_iterator *iter)
+{
+	/* The fmt is either NULL, allocated or points to static_fmt_buf */
+	if (iter->fmt != static_fmt_buf)
+		kfree(iter->fmt);
+
+	kfree(iter->temp);
+	kfree(iter->buffer_iter);
+	mutex_destroy(&iter->mutex);
+	free_cpumask_var(iter->started);
+}
+
 static struct trace_iterator *
 __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 {
@@ -4922,8 +4941,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 
  fail:
 	mutex_unlock(&trace_types_lock);
-	kfree(iter->temp);
-	kfree(iter->buffer_iter);
+	free_trace_iter_content(iter);
 release:
 	seq_release_private(inode, file);
 	return ERR_PTR(-ENOMEM);
@@ -5002,11 +5020,7 @@ static int tracing_release(struct inode *inode, struct file *file)
 
 	mutex_unlock(&trace_types_lock);
 
-	mutex_destroy(&iter->mutex);
-	free_cpumask_var(iter->started);
-	kfree(iter->fmt);
-	kfree(iter->temp);
-	kfree(iter->buffer_iter);
+	free_trace_iter_content(iter);
 	seq_release_private(inode, file);
 
 	return 0;
@@ -6709,7 +6723,12 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
 	}
 
 	trace_seq_init(&iter->seq);
-	iter->trace = tr->current_trace;
+
+	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
+	if (!iter->trace)
+		goto fail;
+
+	*iter->trace = *tr->current_trace;
 
 	if (!alloc_cpumask_var(&iter->started, GFP_KERNEL)) {
 		ret = -ENOMEM;
@@ -6763,10 +6782,7 @@ static int tracing_release_pipe(struct inode *inode, struct file *file)
 
 	mutex_unlock(&trace_types_lock);
 
-	free_cpumask_var(iter->started);
-	kfree(iter->fmt);
-	kfree(iter->temp);
-	mutex_destroy(&iter->mutex);
+	free_trace_iter_content(iter);
 	kfree(iter);
 
 	trace_array_put(tr);
-- 
2.40.1

Re: [PATCH 2/2] tracing: Add free_trace_iter_content() helper function

[Masami Hiramatsu]

On Thu, 13 Jul 2023 11:47:00 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> As the trace iterator is created and used by various interfaces, the clean
> up of it needs to be consistent. Create a free_trace_iter_content() helper
> function that frees the content of the iterator and use that to clean it
> up in all places that it is used.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 1c370ffbe062..3f38250637e2 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4815,6 +4815,25 @@ static const struct seq_operations tracer_seq_ops = {
>  	.show		= s_show,
>  };
>  
> +/*
> + * Note, as iter itself can be allocated and freed in different
> + * ways, this function is only used to free its content, and not
> + * the iterator itself. The only requirement to all the allocations
> + * is that it must zero all fields (kzalloc), as freeing works with
> + * ethier allocated content or NULL.
> + */
> +static void free_trace_iter_content(struct trace_iterator *iter)
> +{
> +	/* The fmt is either NULL, allocated or points to static_fmt_buf */
> +	if (iter->fmt != static_fmt_buf)
> +		kfree(iter->fmt);
> +
> +	kfree(iter->temp);
> +	kfree(iter->buffer_iter);
> +	mutex_destroy(&iter->mutex);
> +	free_cpumask_var(iter->started);

This doesn't kfree(iter->trace) because iter->trace is just a reference
if I understand correctly.

> +}
> +
>  static struct trace_iterator *
>  __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  {
> @@ -4922,8 +4941,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  
>   fail:
>  	mutex_unlock(&trace_types_lock);
> -	kfree(iter->temp);
> -	kfree(iter->buffer_iter);
> +	free_trace_iter_content(iter);
>  release:
>  	seq_release_private(inode, file);
>  	return ERR_PTR(-ENOMEM);
> @@ -5002,11 +5020,7 @@ static int tracing_release(struct inode *inode, struct file *file)
>  
>  	mutex_unlock(&trace_types_lock);
>  
> -	mutex_destroy(&iter->mutex);
> -	free_cpumask_var(iter->started);
> -	kfree(iter->fmt);
> -	kfree(iter->temp);
> -	kfree(iter->buffer_iter);
> +	free_trace_iter_content(iter);
>  	seq_release_private(inode, file);
>  
>  	return 0;
> @@ -6709,7 +6723,12 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
>  	}
>  
>  	trace_seq_init(&iter->seq);
> -	iter->trace = tr->current_trace;
> +
> +	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
> +	if (!iter->trace)
> +		goto fail;
> +
> +	*iter->trace = *tr->current_trace;

Hmm, you allocate iter->trace here (again)

>  
>  	if (!alloc_cpumask_var(&iter->started, GFP_KERNEL)) {
>  		ret = -ENOMEM;
> @@ -6763,10 +6782,7 @@ static int tracing_release_pipe(struct inode *inode, struct file *file)
>  
>  	mutex_unlock(&trace_types_lock);
>  
> -	free_cpumask_var(iter->started);
> -	kfree(iter->fmt);
> -	kfree(iter->temp);
> -	mutex_destroy(&iter->mutex);

But there is no kfree(iter->trace).

Thank you,

> +	free_trace_iter_content(iter);
>  	kfree(iter);
>  
>  	trace_array_put(tr);
> -- 
> 2.40.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 2/2] tracing: Add free_trace_iter_content() helper function

[Steven Rostedt]

On Fri, 14 Jul 2023 17:47:57 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > @@ -6709,7 +6723,12 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
> >  	}
> >  
> >  	trace_seq_init(&iter->seq);
> > -	iter->trace = tr->current_trace;
> > +
> > +	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
> > +	if (!iter->trace)
> > +		goto fail;
> > +
> > +	*iter->trace = *tr->current_trace;  
> 
> Hmm, you allocate iter->trace here (again)

Bah, that looks like it got out of sync with the previous patch (which
removed that). That's not suppose to be there.

I'll fix this an send out a v2. Thanks for catching that!

-- Steve

Re: [PATCH 2/2] tracing: Add free_trace_iter_content() helper function

[Zheng Yejian]

On 2023/7/13 23:47, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> As the trace iterator is created and used by various interfaces, the clean
> up of it needs to be consistent. Create a free_trace_iter_content() helper
> function that frees the content of the iterator and use that to clean it
> up in all places that it is used.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>   kernel/trace/trace.c | 40 ++++++++++++++++++++++++++++------------
>   1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 1c370ffbe062..3f38250637e2 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4815,6 +4815,25 @@ static const struct seq_operations tracer_seq_ops = {
>   	.show		= s_show,
>   };
>   
> +/*
> + * Note, as iter itself can be allocated and freed in different
> + * ways, this function is only used to free its content, and not
> + * the iterator itself. The only requirement to all the allocations
> + * is that it must zero all fields (kzalloc), as freeing works with
> + * ethier allocated content or NULL.
> + */
> +static void free_trace_iter_content(struct trace_iterator *iter)
> +{
> +	/* The fmt is either NULL, allocated or points to static_fmt_buf */
> +	if (iter->fmt != static_fmt_buf)
> +		kfree(iter->fmt);
> +
> +	kfree(iter->temp);
> +	kfree(iter->buffer_iter);
> +	mutex_destroy(&iter->mutex);
> +	free_cpumask_var(iter->started);
> +}
> +
>   static struct trace_iterator *
>   __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>   {
> @@ -4922,8 +4941,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>   
>    fail:
>   	mutex_unlock(&trace_types_lock);
> -	kfree(iter->temp);
> -	kfree(iter->buffer_iter);
> +	free_trace_iter_content(iter);
>   release:
>   	seq_release_private(inode, file);
>   	return ERR_PTR(-ENOMEM);
> @@ -5002,11 +5020,7 @@ static int tracing_release(struct inode *inode, struct file *file)
>   
>   	mutex_unlock(&trace_types_lock);
>   
> -	mutex_destroy(&iter->mutex);
> -	free_cpumask_var(iter->started);
> -	kfree(iter->fmt);
> -	kfree(iter->temp);
> -	kfree(iter->buffer_iter);
> +	free_trace_iter_content(iter);
>   	seq_release_private(inode, file);
>   
>   	return 0;
> @@ -6709,7 +6723,12 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
>   	}
>   
>   	trace_seq_init(&iter->seq);
> -	iter->trace = tr->current_trace;
> +
> +	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
> +	if (!iter->trace)
> +		goto fail;

Hi, Steve, 'ret' may need to be set before `goto fail`:
   ret = -ENOMEM;

> +
> +	*iter->trace = *tr->current_trace;
>   
>   	if (!alloc_cpumask_var(&iter->started, GFP_KERNEL)) {
>   		ret = -ENOMEM;
> @@ -6763,10 +6782,7 @@ static int tracing_release_pipe(struct inode *inode, struct file *file)
>   
>   	mutex_unlock(&trace_types_lock);
>   
> -	free_cpumask_var(iter->started);
> -	kfree(iter->fmt);
> -	kfree(iter->temp);
> -	mutex_destroy(&iter->mutex);
> +	free_trace_iter_content(iter);
>   	kfree(iter);
>   
>   	trace_array_put(tr);

Re: [PATCH 2/2] tracing: Add free_trace_iter_content() helper function

[Steven Rostedt]

On Sat, 15 Jul 2023 13:15:32 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:

> > @@ -6709,7 +6723,12 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
> >   	}
> >   
> >   	trace_seq_init(&iter->seq);
> > -	iter->trace = tr->current_trace;
> > +
> > +	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
> > +	if (!iter->trace)
> > +		goto fail;  
> 
> Hi, Steve, 'ret' may need to be set before `goto fail`:
>    ret = -ENOMEM;
> 

As I mentioned to Masami, this hunk of the patch didn't belong.
Something got mixed up in the commit. This patch even depends on the
previous patch to remove the allocation completely.

-- Steve

Re: [PATCH 2/2] tracing: Add free_trace_iter_content() helper function

[Steven Rostedt]

On Sat, 15 Jul 2023 09:42:01 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 15 Jul 2023 13:15:32 +0800
> Zheng Yejian <zhengyejian1@huawei.com> wrote:
> 
> > > @@ -6709,7 +6723,12 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
> > >   	}
> > >   
> > >   	trace_seq_init(&iter->seq);
> > > -	iter->trace = tr->current_trace;
> > > +
> > > +	iter->trace = kzalloc(sizeof(*iter->trace), GFP_KERNEL);
> > > +	if (!iter->trace)
> > > +		goto fail;  
> > 
> > Hi, Steve, 'ret' may need to be set before `goto fail`:
> >    ret = -ENOMEM;
> > 
> 
> As I mentioned to Masami, this hunk of the patch didn't belong.
> Something got mixed up in the commit. This patch even depends on the
> previous patch to remove the allocation completely.
> 

I know what I did now. I first started writing this patch and saw that
the iter->trace was inconsistent and in one place allocated a copy and
here it did not. I started to make the copy here, when I realized that
there was no reason to make that copy. Then I wrote the first patch to
remove the copying, but forgot to remove the copying I added in this
patch! :-p

-- Steve