| Message ID | 20230713114700.450e7a17@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing: Clean up how iter is freed | expand |
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 >
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
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);
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
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 --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);