tracing: Miscellaneous fixes for trace_array ref counting
PATCH 1/2 tracing: Miscellaneous fixes for trace_array ref counting
Some error paths did not handle ref counting properly, and some trace files need ref counting.
--- kernel/trace/trace.c | 34 +++++++++++++++++++++++----------- kernel/trace/trace_events.c | 21 +++++++++++++++++++-- 2 files changed, 42 insertions(+), 13 deletions(-)
kernel/trace/trace.c +23 -11
@@ -3008,7 +3008,6 @@ static int tracing_release(struct inode *inode, struct file *file) iter = m->private; tr = iter->tr; -trace_array_put (tr);mutex_lock (&trace_types_lock);
@@ -3023,6 +3022,9 @@ static int tracing_release(struct inode *inode, struct file *file)if (!iter->snapshot)/* reenable tracing if it was previously enabled */ tracing_start_tr (tr); + +__trace_array_put (tr); +mutex_unlock (&trace_types_lock);mutex_destroy (&iter->mutex);
@@ -3447,6 +3449,7 @@ tracing_trace_options_write(struct file *filp, const char __user *ubuf,static int tracing_trace_options_open (struct inode *inode,struct file *file) {struct trace_array *tr = inode->i_private; +int ret;if (tracing_disabled)return -ENODEV ;
@@ -3454,7 +3457,11 @@ static int tracing_trace_options_open(struct inode *inode, struct file *file)if (trace_array_get (tr) <0 )return -ENODEV ; -return single_open (file, tracing_trace_options_show, inode->i_private); + ret =single_open (file, tracing_trace_options_show, inode->i_private); +if (ret <0 ) +trace_array_put (tr); + +return ret; }static const struct file_operations tracing_iter_fops = {
@@ -3958,7 +3965,7 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) iter =kzalloc (sizeof (*iter),GFP_KERNEL );if (!iter) { ret = -ENOMEM ; - goto out; + goto fail_iter; }/*
@@ -3968,13 +3975,13 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) iter->trace =kmalloc (sizeof (*iter->trace),GFP_KERNEL );if (!iter->trace) { ret = -ENOMEM ; - goto fail; + goto fail_trace; } *iter->trace = *tr->current_trace;if (!alloc_cpumask_var (&iter->started,GFP_KERNEL )) { ret = -ENOMEM ; - goto fail; + goto fail_trace; }/* trace pipe does not show start of buffer */
@@ -4001,9 +4008,10 @@ out:mutex_unlock (&trace_types_lock);return ret; -fail: +fail_trace:kfree (iter->trace);kfree (iter); +fail_iter:__trace_array_put (tr);mutex_unlock (&trace_types_lock);return ret;
@@ -4705,12 +4713,15 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file) }else {/* Writes still need the seq_file to hold the private data */ m =kzalloc (sizeof (*m),GFP_KERNEL ); -if (!m) -return -ENOMEM ; +if (!m) { + ret = -ENOMEM ; + goto out; + } iter =kzalloc (sizeof (*iter),GFP_KERNEL );if (!iter) {kfree (m); -return -ENOMEM ; + ret = -ENOMEM ; + goto out; } iter->tr = tr; iter->trace_buffer = &tc->tr->max_buffer;
@@ -4718,7 +4729,7 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file) m->private = iter; file->private_data = m; } - +out:if (ret <0 )trace_array_put (tr);
@@ -5328,9 +5339,10 @@ tracing_stats_read(struct file *filp, char __user *ubuf, }static const struct file_operations tracing_stats_fops = { - .open = tracing_open_generic, + .open = tracing_open_generic_tc, .read = tracing_stats_read, .llseek = generic_file_llseek, + .release = tracing_release_generic_tc, };#ifdef CONFIG_DYNAMIC_FTRACE
kernel/trace/trace_events.c +19 -2
@@ -1218,6 +1218,7 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)static int ftrace_event_avail_open (struct inode *inode,struct file *file);static int ftrace_event_set_open (struct inode *inode,struct file *file); +static int ftrace_event_release (struct inode *inode,struct file *file);static const struct seq_operations show_event_seq_ops = { .start = t_start,
@@ -1245,7 +1246,7 @@ static const struct file_operations ftrace_set_event_fops = { .read = seq_read, .write = ftrace_event_write, .llseek = seq_lseek, - .release = seq_release, + .release = ftrace_event_release, };static const struct file_operations ftrace_enable_fops = {
@@ -1323,6 +1324,15 @@ ftrace_event_open(struct inode *inode, struct file *file,return ret; } +static int ftrace_event_release (struct inode *inode,struct file *file) +{ +struct trace_array *tr = inode->i_private; + +trace_array_put (tr); + +return seq_release (inode, file); +} +static int ftrace_event_avail_open (struct inode *inode,struct file *file) {
@@ -1336,12 +1346,19 @@ ftrace_event_set_open(struct inode *inode, struct file *file) {const struct seq_operations *seq_ops = &show_set_event_seq_ops;struct trace_array *tr = inode->i_private; +int ret; + +if (trace_array_get (tr) <0 ) +return -ENODEV ;if ((file->f_mode &FMODE_WRITE ) && (file->f_flags &O_TRUNC ))ftrace_clear_events (tr); -return ftrace_event_open (inode, file, seq_ops); + ret =ftrace_event_open (inode, file, seq_ops); +if (ret <0 ) +trace_array_put (tr); +return ret; } static struct event_subsystem *
Signature · 1 line
1.8.3
PATCH 2/2 tracing: Fix error handling to ensure instances can always be removed
Remove debugfs directories for tracing instances during creation if an error occurs causing the trace_array for that instance to not be added to ftrace_trace_arrays. If the directory continues to exist after the error, it cannot be removed because the respective trace_array is not in ftrace_trace_arrays.
--- kernel/trace/trace.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/trace/trace.c +3 -1
@@ -5985,8 +5985,10 @@ static int new_instance_create(const char *name) goto out_free_tr; ret =event_trace_add_tracer (tr->dir, tr); -if (ret) +if (ret) { +debugfs_remove_recursive (tr->dir); goto out_free_tr; + }init_tracer_debugfs (tr, tr->dir);
Signature · 1 line
1.8.3
Steven Rostedt
maintainerRe: [PATCH 2/2] tracing: Fix error handling to ensure instances can always be removed
Quoted 5 lines · Alexander Z Lam
Remove debugfs directories for tracing instances during creation if an error occurs causing the
trace_arrayfor that instance to not be added toftrace_trace_arrays. If the directory continues to exist after the error, it cannot be removed because the respectivetrace_arrayis not inftrace_trace_arrays.
This patch is fine, I'll put it in now.
Thanks,
-- Steve
Quoted 25 lines
Cc Vaibhav Nagarnaik <[email protected]>Cc David Sharp <[email protected]>Cc Alexander Z Lam <[email protected]>Signed-off-by Alexander Z Lam <[email protected]>--- kernel/trace/trace.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)kernel/trace/trace.c +3
-1@@ -5985,8 +5985,10 @@ static int new_instance_create(const char *name) goto out_free_tr; ret =event_trace_add_tracer (tr->dir, tr); -if (ret) +if (ret) { +debugfs_remove_recursive (tr->dir); goto out_free_tr; + }init_tracer_debugfs (tr, tr->dir);
Steven Rostedt
maintainerQuoted 56 lines · Alexander Z Lam
Some error paths did not handle ref counting properly, and some trace files need ref counting.
Cc Vaibhav Nagarnaik <[email protected]>Cc David Sharp <[email protected]>Cc Alexander Z Lam <[email protected]>Signed-off-by Alexander Z Lam <[email protected]>--- kernel/trace/trace.c | 34 +++++++++++++++++++++++----------- kernel/trace/trace_events.c | 21 +++++++++++++++++++-- 2 files changed, 42 insertions(+), 13 deletions(-)kernel/trace/trace.c +9
-2@@ -3008,7 +3008,6 @@ static int tracing_release(struct inode *inode, struct file *file) iter = m->private; tr = iter->tr; -trace_array_put (tr);mutex_lock (&trace_types_lock);@@ -3023,6 +3022,9 @@ static int tracing_release(struct inode *inode, struct file *file)if (!iter->snapshot)/* reenable tracing if it was previously enabled */ tracing_start_tr (tr); + +__trace_array_put (tr); +mutex_unlock (&trace_types_lock);mutex_destroy (&iter->mutex);@@ -3447,6 +3449,7 @@ tracing_trace_options_write(struct file *filp, const char __user *ubuf,static int tracing_trace_options_open (struct inode *inode,struct file *file) {struct trace_array *tr = inode->i_private; +int ret;if (tracing_disabled)return -ENODEV ;@@ -3454,7 +3457,11 @@ static int tracing_trace_options_open(struct inode *inode, struct file *file)if (trace_array_get (tr) <0 )return -ENODEV ; -return single_open (file, tracing_trace_options_show, inode->i_private); + ret =single_open (file, tracing_trace_options_show, inode->i_private); +if (ret <0 ) +trace_array_put (tr); + +return ret; }static const struct file_operations tracing_iter_fops = {
kernel/trace/trace.c static int tracing_open_pipe(struct inode *inode, struct file *filp) · line 3968
Quoted 5 lines
kernel/trace/trace.c +1
-1@@ -3958,7 +3965,7 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) iter =kzalloc (sizeof (*iter),GFP_KERNEL );if (!iter) { ret = -ENOMEM ; - goto out; + goto fail_iter;
It would be cleaner to not add the extra label, and just do a trace_array_put() in the if statement.
Quoted 32 lines
kernel/trace/trace.c +4
-3}/* @@ -3968,13 +3975,13 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) iter->trace =kmalloc (sizeof (*iter->trace),GFP_KERNEL );if (!iter->trace) { ret = -ENOMEM ; - goto fail; + goto fail_trace; } *iter->trace = *tr->current_trace;if (!alloc_cpumask_var (&iter->started,GFP_KERNEL )) { ret = -ENOMEM ; - goto fail; + goto fail_trace; }/* trace pipe does not show start of buffer */ @@ -4001,9 +4008,10 @@ out:mutex_unlock (&trace_types_lock);return ret; -fail: +fail_trace:kfree (iter->trace);kfree (iter); +fail_iter:__trace_array_put (tr);mutex_unlock (&trace_types_lock);return ret;
kernel/trace/trace.c static int tracing_snapshot_open(struct inode *inode, struct file *file) · line 4719
Quoted 9 lines
kernel/trace/trace.c +4
-2@@ -4705,12 +4713,15 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file) }else {/* Writes still need the seq_file to hold the private data */ m =kzalloc (sizeof (*m),GFP_KERNEL ); -if (!m) -return -ENOMEM ; +if (!m) { + ret = -ENOMEM ; + goto out; + }
Instead of adding the complex if statement, do this:
int ret = -ENOMEM; [..] if (!m) goto out;
And then after the if statements:
ret = 0;
Quoted 10 lines
kernel/trace/trace.c +2
-1iter =kzalloc (sizeof (*iter),GFP_KERNEL );if (!iter) {kfree (m); -return -ENOMEM ; + ret = -ENOMEM ; + goto out; } iter->tr = tr; iter->trace_buffer = &tc->tr->max_buffer;
kernel/trace/trace.c static int tracing_snapshot_open(struct inode *inode, struct file *file) · line 4735
Quoted 8 lines
kernel/trace/trace.c +1
-1@@ -4718,7 +4729,7 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file) m->private = iter; file->private_data = m; } - +out:if (ret <0 )trace_array_put (tr);
The rest looks good. Although, we may be rewriting all this code because there still exists some races. See the email threads between Oleg and Masami.
-- Steve
Quoted 70 lines
kernel/trace/trace.c +2
-1@@ -5328,9 +5339,10 @@ tracing_stats_read(struct file *filp, char __user *ubuf, }static const struct file_operations tracing_stats_fops = { - .open = tracing_open_generic, + .open = tracing_open_generic_tc, .read = tracing_stats_read, .llseek = generic_file_llseek, + .release = tracing_release_generic_tc, };#ifdef CONFIG_DYNAMIC_FTRACE kernel/trace/trace_events.c +19
-2@@ -1218,6 +1218,7 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)static int ftrace_event_avail_open (struct inode *inode,struct file *file);static int ftrace_event_set_open (struct inode *inode,struct file *file); +static int ftrace_event_release (struct inode *inode,struct file *file);static const struct seq_operations show_event_seq_ops = { .start = t_start,@@ -1245,7 +1246,7 @@ static const struct file_operations ftrace_set_event_fops = { .read = seq_read, .write = ftrace_event_write, .llseek = seq_lseek, - .release = seq_release, + .release = ftrace_event_release, };static const struct file_operations ftrace_enable_fops = {@@ -1323,6 +1324,15 @@ ftrace_event_open(struct inode *inode, struct file *file,return ret; } +static int ftrace_event_release (struct inode *inode,struct file *file) +{ +struct trace_array *tr = inode->i_private; + +trace_array_put (tr); + +return seq_release (inode, file); +} +static int ftrace_event_avail_open (struct inode *inode,struct file *file) {@@ -1336,12 +1346,19 @@ ftrace_event_set_open(struct inode *inode, struct file *file) {const struct seq_operations *seq_ops = &show_set_event_seq_ops;struct trace_array *tr = inode->i_private; +int ret; + +if (trace_array_get (tr) <0 ) +return -ENODEV ;if ((file->f_mode &FMODE_WRITE ) && (file->f_flags &O_TRUNC ))ftrace_clear_events (tr); -return ftrace_event_open (inode, file, seq_ops); + ret =ftrace_event_open (inode, file, seq_ops); +if (ret <0 ) +trace_array_put (tr); +return ret; } static struct event_subsystem *