Skip to content

Commit

Permalink
tracing: Remove unnecessary copying of tr->current_trace
Browse files Browse the repository at this point in the history
The iterator allocated a descriptor to copy the current_trace. This was done
with the assumption that the function pointers might change. But this was a
false assuption, as it does not change. There's no reason to make a copy of the
current_trace and just use the pointer it points to. This removes needing to
manage freeing the descriptor. Worse yet, there's locations that the iterator
is used but does make a copy and just uses the pointer. This could cause the
actual pointer to the trace descriptor to be freed and not the allocated copy.

This is more of a clean up than a fix.

Link: https://lkml.kernel.org/r/20230715141348.135792275@goodmis.org

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Fixes: d7350c3 ("tracing/core: make the read callbacks reentrants")
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
  • Loading branch information
Steven Rostedt (Google) committed Jul 30, 2023
1 parent 00a8478 commit 9182b51
Showing 1 changed file with 3 additions and 19 deletions.
22 changes: 3 additions & 19 deletions kernel/trace/trace.c
Original file line number Diff line number Diff line change
Expand Up @@ -4189,15 +4189,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
Expand Down Expand Up @@ -4846,16 +4840,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;
Expand Down Expand Up @@ -4920,7 +4906,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:
Expand Down Expand Up @@ -5005,7 +4990,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);

Expand Down

0 comments on commit 9182b51

Please sign in to comment.