diff mbox series

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

Message ID 20230713114700.450e7a17@gandalf.local.home (mailing list archive)
State Superseded
Headers show
Series tracing: Clean up how iter is freed | expand

Commit Message

Steven Rostedt July 13, 2023, 3:47 p.m. UTC
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(-)

Comments

Masami Hiramatsu (Google) July 14, 2023, 8:47 a.m. UTC | #1
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
>
Steven Rostedt July 14, 2023, 2:22 p.m. UTC | #2
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
Zheng Yejian July 15, 2023, 5:15 a.m. UTC | #3
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);
Steven Rostedt July 15, 2023, 1:42 p.m. UTC | #4
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
Steven Rostedt July 15, 2023, 1:54 p.m. UTC | #5
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
diff mbox series

Patch

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);