From: "Steven Rostedt (Google)" <[email protected]>
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) <[email protected]>
---
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
On Thu, 13 Jul 2023 11:47:00 -0400
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> 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) <[email protected]>
> ---
> 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) <[email protected]>
On Fri, 14 Jul 2023 17:47:57 +0900
Masami Hiramatsu (Google) <[email protected]> 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)" <[email protected]>
>
> 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) <[email protected]>
> ---
> 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 09:42:01 -0400
Steven Rostedt <[email protected]> wrote:
> On Sat, 15 Jul 2023 13:15:32 +0800
> Zheng Yejian <[email protected]> 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
On Sat, 15 Jul 2023 13:15:32 +0800
Zheng Yejian <[email protected]> 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