[PATCH 0/7] tracing: Kill the buggy trace_cpu

[PATCH 0/7] tracing: Kill the buggy trace_cpu

[Oleg Nesterov]

Hello.

Only slightly tested so far. I'll try to test more and report
the results.

Meanwhile it would be nice if you can take a look and review.


This series kills tracing_open_generic_tc/trace_cpu/etc.

trace_array_get(inode->i_private) is mostly fine, we do not
dereference this pointer untill trace_array_get() succeeds.
But trace_array_get(tc->tr) is wrong by the same reason why
tracing_open_generic_file/etc are wrong, see 1/7.

And not only trace_cpu is buggy, we can simplify the code if
we kill this member.

Oleg.

 kernel/trace/trace.c |  192 ++++++++++++++++++++------------------------------
 kernel/trace/trace.h |    8 --
 2 files changed, 76 insertions(+), 124 deletions(-)

[PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Oleg Nesterov]

Every "file_operations" used by tracing_init_debugfs_percpu is buggy.
f_op->open/etc does:

	1. struct trace_cpu *tc = inode->i_private;
	   struct trace_array *tr = tc->tr;

	2. trace_array_get(tr) or fail;

	3. do_something(tc);

But tc (and tr) can be already freed before trace_array_get() is called.
And it doesn't matter whether this file is per-cpu or it was created by
init_tracer_debugfs(), free_percpu() or kfree() are equally bad.

Note that even 1. is not safe, the freed memory can be unmapped. But even
if it was safe trace_array_get() can wrongly succeed if we also race with
the next new_instance_create() which can re-allocate the same tr, or tc
was overwritten and ->tr points to the valid tr. In this case 3. uses the
freed/reused memory.

Add the new trivial helper, trace_create_cpu_file() which simply calls
trace_create_file() and encodes "cpu" in "struct inode". Another helper,
tracing_get_cpu() will be used to read cpu_nr-or-RING_BUFFER_ALL_CPUS.

The patch abuses ->i_cdev to encode the number, it is never used unless
the file is S_ISCHR(). But we could use something else, say, i_bytes or
even ->d_fsdata. In any case this hack is hidden inside these 2 helpers,
it would be trivial to change them if needed.

This patch only changes tracing_init_debugfs_percpu() to use the new
trace_create_cpu_file(), the next patches will change file_operations.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace.c |   46 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 3f24777..1e0fae9 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2843,6 +2843,13 @@ static int s_show(struct seq_file *m, void *v)
 	return 0;
 }
 
+static inline int tracing_get_cpu(struct inode *inode)
+{
+	if (inode->i_cdev) /* See trace_create_cpu_file() */
+		return (long)inode->i_cdev - 1;
+	return RING_BUFFER_ALL_CPUS;
+}
+
 static const struct seq_operations tracer_seq_ops = {
 	.start		= s_start,
 	.next		= s_next,
@@ -5529,6 +5536,17 @@ static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu)
 	return tr->percpu_dir;
 }
 
+static struct dentry *
+trace_create_cpu_file(const char *name, umode_t mode, struct dentry *parent,
+		      void *data, long cpu, const struct file_operations *fops)
+{
+	struct dentry *ret = trace_create_file(name, mode, parent, data, fops);
+
+	if (ret) /* See tracing_get_cpu() */
+		ret->d_inode->i_cdev = (void*)(cpu + 1);
+	return ret;
+}
+
 static void
 tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 {
@@ -5548,28 +5566,28 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 	}
 
 	/* per cpu trace_pipe */
-	trace_create_file("trace_pipe", 0444, d_cpu,
-			(void *)&data->trace_cpu, &tracing_pipe_fops);
+	trace_create_cpu_file("trace_pipe", 0444, d_cpu,
+				&data->trace_cpu, cpu, &tracing_pipe_fops);
 
 	/* per cpu trace */
-	trace_create_file("trace", 0644, d_cpu,
-			(void *)&data->trace_cpu, &tracing_fops);
+	trace_create_cpu_file("trace", 0644, d_cpu,
+				&data->trace_cpu, cpu, &tracing_fops);
 
-	trace_create_file("trace_pipe_raw", 0444, d_cpu,
-			(void *)&data->trace_cpu, &tracing_buffers_fops);
+	trace_create_cpu_file("trace_pipe_raw", 0444, d_cpu,
+				&data->trace_cpu, cpu, &tracing_buffers_fops);
 
-	trace_create_file("stats", 0444, d_cpu,
-			(void *)&data->trace_cpu, &tracing_stats_fops);
+	trace_create_cpu_file("stats", 0444, d_cpu,
+				&data->trace_cpu, cpu, &tracing_stats_fops);
 
-	trace_create_file("buffer_size_kb", 0444, d_cpu,
-			(void *)&data->trace_cpu, &tracing_entries_fops);
+	trace_create_cpu_file("buffer_size_kb", 0444, d_cpu,
+				&data->trace_cpu, cpu, &tracing_entries_fops);
 
 #ifdef CONFIG_TRACER_SNAPSHOT
-	trace_create_file("snapshot", 0644, d_cpu,
-			  (void *)&data->trace_cpu, &snapshot_fops);
+	trace_create_cpu_file("snapshot", 0644, d_cpu,
+			  	&data->trace_cpu, cpu, &snapshot_fops);
 
-	trace_create_file("snapshot_raw", 0444, d_cpu,
-			(void *)&data->trace_cpu, &snapshot_raw_fops);
+	trace_create_cpu_file("snapshot_raw", 0444, d_cpu,
+				&data->trace_cpu, cpu, &snapshot_raw_fops);
 #endif
 }
 
-- 
1.5.5.1

[PATCH 2/7] tracing: Change tracing_pipe_fops() to rely on tracing_get_cpu()

[Oleg Nesterov]

tracing_open_pipe() is racy, the memory inode->i_private points to
can be already freed.

Change debugfs_create_file("trace_pipe", data) callers to to pass
"data = tr", tracing_open_pipe() can use tracing_get_cpu().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 1e0fae9..311581a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3955,8 +3955,7 @@ tracing_max_lat_write(struct file *filp, const char __user *ubuf,
 
 static int tracing_open_pipe(struct inode *inode, struct file *filp)
 {
-	struct trace_cpu *tc = inode->i_private;
-	struct trace_array *tr = tc->tr;
+	struct trace_array *tr = inode->i_private;
 	struct trace_iterator *iter;
 	int ret = 0;
 
@@ -4002,9 +4001,9 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
 	if (trace_clocks[tr->clock_id].in_ns)
 		iter->iter_flags |= TRACE_FILE_TIME_IN_NS;
 
-	iter->cpu_file = tc->cpu;
-	iter->tr = tc->tr;
-	iter->trace_buffer = &tc->tr->trace_buffer;
+	iter->tr = tr;
+	iter->trace_buffer = &tr->trace_buffer;
+	iter->cpu_file = tracing_get_cpu(inode);
 	mutex_init(&iter->mutex);
 	filp->private_data = iter;
 
@@ -4027,8 +4026,7 @@ fail:
 static int tracing_release_pipe(struct inode *inode, struct file *file)
 {
 	struct trace_iterator *iter = file->private_data;
-	struct trace_cpu *tc = inode->i_private;
-	struct trace_array *tr = tc->tr;
+	struct trace_array *tr = inode->i_private;
 
 	mutex_lock(&trace_types_lock);
 
@@ -5567,7 +5565,7 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 
 	/* per cpu trace_pipe */
 	trace_create_cpu_file("trace_pipe", 0444, d_cpu,
-				&data->trace_cpu, cpu, &tracing_pipe_fops);
+				tr, cpu, &tracing_pipe_fops);
 
 	/* per cpu trace */
 	trace_create_cpu_file("trace", 0644, d_cpu,
@@ -6153,7 +6151,7 @@ init_tracer_debugfs(struct trace_array *tr, struct dentry *d_tracer)
 			(void *)&tr->trace_cpu, &tracing_fops);
 
 	trace_create_file("trace_pipe", 0444, d_tracer,
-			(void *)&tr->trace_cpu, &tracing_pipe_fops);
+			  tr, &tracing_pipe_fops);
 
 	trace_create_file("buffer_size_kb", 0644, d_tracer,
 			(void *)&tr->trace_cpu, &tracing_entries_fops);
-- 
1.5.5.1

[PATCH 3/7] tracing: Change tracing_buffers_fops to rely on tracing_get_cpu()

[Oleg Nesterov]

tracing_buffers_open() is racy, the memory inode->i_private points
to can be already freed.

Change debugfs_create_file("trace_pipe_raw", data) caller to pass
"data = tr", tracing_buffers_open() can use tracing_get_cpu().

Change debugfs_create_file("snapshot_raw_fops", data) caller too,
this file uses tracing_buffers_open/release.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 311581a..5a7fbc3 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4945,8 +4945,7 @@ static const struct file_operations snapshot_raw_fops = {
 
 static int tracing_buffers_open(struct inode *inode, struct file *filp)
 {
-	struct trace_cpu *tc = inode->i_private;
-	struct trace_array *tr = tc->tr;
+	struct trace_array *tr = inode->i_private;
 	struct ftrace_buffer_info *info;
 	int ret;
 
@@ -4965,7 +4964,7 @@ static int tracing_buffers_open(struct inode *inode, struct file *filp)
 	mutex_lock(&trace_types_lock);
 
 	info->iter.tr		= tr;
-	info->iter.cpu_file	= tc->cpu;
+	info->iter.cpu_file	= tracing_get_cpu(inode);
 	info->iter.trace	= tr->current_trace;
 	info->iter.trace_buffer = &tr->trace_buffer;
 	info->spare		= NULL;
@@ -5572,7 +5571,7 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 				&data->trace_cpu, cpu, &tracing_fops);
 
 	trace_create_cpu_file("trace_pipe_raw", 0444, d_cpu,
-				&data->trace_cpu, cpu, &tracing_buffers_fops);
+				tr, cpu, &tracing_buffers_fops);
 
 	trace_create_cpu_file("stats", 0444, d_cpu,
 				&data->trace_cpu, cpu, &tracing_stats_fops);
@@ -5585,7 +5584,7 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 			  	&data->trace_cpu, cpu, &snapshot_fops);
 
 	trace_create_cpu_file("snapshot_raw", 0444, d_cpu,
-				&data->trace_cpu, cpu, &snapshot_raw_fops);
+				tr, cpu, &snapshot_raw_fops);
 #endif
 }
 
-- 
1.5.5.1

[PATCH 4/7] tracing: Change tracing_stats_fops to rely on tracing_get_cpu()

[Oleg Nesterov]

tracing_open_generic_tc() is racy, the memory inode->i_private
points to can be already freed.

1. Change one of its users, tracing_stats_fops, to use
   tracing_*_generic_tr() instead.

2. Change trace_create_cpu_file("stats", data) to pass "data = tr".

3. Change tracing_stats_read() to use tracing_get_cpu().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 5a7fbc3..d6429e5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2978,7 +2978,6 @@ static int tracing_open_generic_tr(struct inode *inode, struct file *filp)
 	filp->private_data = inode->i_private;
 
 	return 0;
-	
 }
 
 static int tracing_open_generic_tc(struct inode *inode, struct file *filp)
@@ -5281,14 +5280,14 @@ static ssize_t
 tracing_stats_read(struct file *filp, char __user *ubuf,
 		   size_t count, loff_t *ppos)
 {
-	struct trace_cpu *tc = filp->private_data;
-	struct trace_array *tr = tc->tr;
+	struct inode *inode = file_inode(filp);
+	struct trace_array *tr = inode->i_private;
 	struct trace_buffer *trace_buf = &tr->trace_buffer;
+	int cpu = tracing_get_cpu(inode);
 	struct trace_seq *s;
 	unsigned long cnt;
 	unsigned long long t;
 	unsigned long usec_rem;
-	int cpu = tc->cpu;
 
 	s = kmalloc(sizeof(*s), GFP_KERNEL);
 	if (!s)
@@ -5341,10 +5340,10 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 }
 
 static const struct file_operations tracing_stats_fops = {
-	.open		= tracing_open_generic_tc,
+	.open		= tracing_open_generic_tr,
 	.read		= tracing_stats_read,
 	.llseek		= generic_file_llseek,
-	.release	= tracing_release_generic_tc,
+	.release	= tracing_release_generic_tr,
 };
 
 #ifdef CONFIG_DYNAMIC_FTRACE
@@ -5574,7 +5573,7 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 				tr, cpu, &tracing_buffers_fops);
 
 	trace_create_cpu_file("stats", 0444, d_cpu,
-				&data->trace_cpu, cpu, &tracing_stats_fops);
+				tr, cpu, &tracing_stats_fops);
 
 	trace_create_cpu_file("buffer_size_kb", 0444, d_cpu,
 				&data->trace_cpu, cpu, &tracing_entries_fops);
-- 
1.5.5.1

[PATCH 5/7] tracing: Change tracing_entries_fops to rely on tracing_get_cpu()

[Oleg Nesterov]

tracing_open_generic_tc() is racy, the memory inode->i_private
points to can be already freed.

1. Change its last user, tracing_entries_fops, to use
   tracing_*_generic_tr() instead.

2. Change debugfs_create_file("buffer_size_kb", data) callers
   to pass "data = tr".

3. Change tracing_entries_read() and tracing_entries_write() to
   use tracing_get_cpu().

4. Kill the no longer used tracing_open_generic_tc() and
   tracing_release_generic_tc().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace.c |   49 ++++++++++++-------------------------------------
 1 files changed, 12 insertions(+), 37 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d6429e5..611113d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2980,23 +2980,6 @@ static int tracing_open_generic_tr(struct inode *inode, struct file *filp)
 	return 0;
 }
 
-static int tracing_open_generic_tc(struct inode *inode, struct file *filp)
-{
-	struct trace_cpu *tc = inode->i_private;
-	struct trace_array *tr = tc->tr;
-
-	if (tracing_disabled)
-		return -ENODEV;
-
-	if (trace_array_get(tr) < 0)
-		return -ENODEV;
-
-	filp->private_data = inode->i_private;
-
-	return 0;
-	
-}
-
 static int tracing_release(struct inode *inode, struct file *file)
 {
 	struct seq_file *m = file->private_data;
@@ -3050,15 +3033,6 @@ static int tracing_release_generic_tr(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int tracing_release_generic_tc(struct inode *inode, struct file *file)
-{
-	struct trace_cpu *tc = inode->i_private;
-	struct trace_array *tr = tc->tr;
-
-	trace_array_put(tr);
-	return 0;
-}
-
 static int tracing_single_release_tr(struct inode *inode, struct file *file)
 {
 	struct trace_array *tr = inode->i_private;
@@ -4378,15 +4352,16 @@ static ssize_t
 tracing_entries_read(struct file *filp, char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
 {
-	struct trace_cpu *tc = filp->private_data;
-	struct trace_array *tr = tc->tr;
+	struct inode *inode = file_inode(filp);
+	struct trace_array *tr = inode->i_private;
+	int cpu = tracing_get_cpu(inode);
 	char buf[64];
 	int r = 0;
 	ssize_t ret;
 
 	mutex_lock(&trace_types_lock);
 
-	if (tc->cpu == RING_BUFFER_ALL_CPUS) {
+	if (cpu == RING_BUFFER_ALL_CPUS) {
 		int cpu, buf_size_same;
 		unsigned long size;
 
@@ -4413,7 +4388,7 @@ tracing_entries_read(struct file *filp, char __user *ubuf,
 		} else
 			r = sprintf(buf, "X\n");
 	} else
-		r = sprintf(buf, "%lu\n", per_cpu_ptr(tr->trace_buffer.data, tc->cpu)->entries >> 10);
+		r = sprintf(buf, "%lu\n", per_cpu_ptr(tr->trace_buffer.data, cpu)->entries >> 10);
 
 	mutex_unlock(&trace_types_lock);
 
@@ -4425,7 +4400,8 @@ static ssize_t
 tracing_entries_write(struct file *filp, const char __user *ubuf,
 		      size_t cnt, loff_t *ppos)
 {
-	struct trace_cpu *tc = filp->private_data;
+	struct inode *inode = file_inode(filp);
+	struct trace_array *tr = inode->i_private;
 	unsigned long val;
 	int ret;
 
@@ -4439,8 +4415,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 
 	/* value is in KB */
 	val <<= 10;
-
-	ret = tracing_resize_ring_buffer(tc->tr, val, tc->cpu);
+	ret = tracing_resize_ring_buffer(tr, val, tracing_get_cpu(inode));
 	if (ret < 0)
 		return ret;
 
@@ -4888,11 +4863,11 @@ static const struct file_operations tracing_pipe_fops = {
 };
 
 static const struct file_operations tracing_entries_fops = {
-	.open		= tracing_open_generic_tc,
+	.open		= tracing_open_generic_tr,
 	.read		= tracing_entries_read,
 	.write		= tracing_entries_write,
 	.llseek		= generic_file_llseek,
-	.release	= tracing_release_generic_tc,
+	.release	= tracing_release_generic_tr,
 };
 
 static const struct file_operations tracing_total_entries_fops = {
@@ -5576,7 +5551,7 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 				tr, cpu, &tracing_stats_fops);
 
 	trace_create_cpu_file("buffer_size_kb", 0444, d_cpu,
-				&data->trace_cpu, cpu, &tracing_entries_fops);
+				tr, cpu, &tracing_entries_fops);
 
 #ifdef CONFIG_TRACER_SNAPSHOT
 	trace_create_cpu_file("snapshot", 0644, d_cpu,
@@ -6152,7 +6127,7 @@ init_tracer_debugfs(struct trace_array *tr, struct dentry *d_tracer)
 			  tr, &tracing_pipe_fops);
 
 	trace_create_file("buffer_size_kb", 0644, d_tracer,
-			(void *)&tr->trace_cpu, &tracing_entries_fops);
+			  tr, &tracing_entries_fops);
 
 	trace_create_file("buffer_total_size_kb", 0444, d_tracer,
 			  tr, &tracing_total_entries_fops);
-- 
1.5.5.1

[PATCH 6/7] tracing: Change tracing_fops/snapshot_fops to rely on tracing_get_cpu()

[Oleg Nesterov]

tracing_open() and tracing_snapshot_open() are racy, the memory
inode->i_private points to can be already freed.

Convert these last users of "inode->i_private == trace_cpu" to
use "i_private = trace_array" and rely on tracing_get_cpu().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace.c |   52 +++++++++++++++++++++----------------------------
 1 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 611113d..0c0b4d5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2858,9 +2858,9 @@ static const struct seq_operations tracer_seq_ops = {
 };
 
 static struct trace_iterator *
-__tracing_open(struct trace_array *tr, struct trace_cpu *tc,
-	       struct inode *inode, struct file *file, bool snapshot)
+__tracing_open(struct inode *inode, struct file *file, bool snapshot)
 {
+	struct trace_array *tr = inode->i_private;
 	struct trace_iterator *iter;
 	int cpu;
 
@@ -2901,8 +2901,8 @@ __tracing_open(struct trace_array *tr, struct trace_cpu *tc,
 		iter->trace_buffer = &tr->trace_buffer;
 	iter->snapshot = snapshot;
 	iter->pos = -1;
+	iter->cpu_file = tracing_get_cpu(inode);
 	mutex_init(&iter->mutex);
-	iter->cpu_file = tc->cpu;
 
 	/* Notify the tracer early; before we stop tracing. */
 	if (iter->trace && iter->trace->open)
@@ -2982,22 +2982,16 @@ static int tracing_open_generic_tr(struct inode *inode, struct file *filp)
 
 static int tracing_release(struct inode *inode, struct file *file)
 {
+	struct trace_array *tr = inode->i_private;
 	struct seq_file *m = file->private_data;
-	struct trace_iterator *iter;
-	struct trace_array *tr;
+	struct trace_iterator *iter = m->private;
 	int cpu;
 
-	/* Writes do not use seq_file, need to grab tr from inode */
 	if (!(file->f_mode & FMODE_READ)) {
-		struct trace_cpu *tc = inode->i_private;
-
-		trace_array_put(tc->tr);
+		trace_array_put(tr);
 		return 0;
 	}
 
-	iter = m->private;
-	tr = iter->tr;
-
 	mutex_lock(&trace_types_lock);
 
 	for_each_tracing_cpu(cpu) {
@@ -3044,8 +3038,7 @@ static int tracing_single_release_tr(struct inode *inode, struct file *file)
 
 static int tracing_open(struct inode *inode, struct file *file)
 {
-	struct trace_cpu *tc = inode->i_private;
-	struct trace_array *tr = tc->tr;
+	struct trace_array *tr = inode->i_private;
 	struct trace_iterator *iter;
 	int ret = 0;
 
@@ -3053,16 +3046,17 @@ static int tracing_open(struct inode *inode, struct file *file)
 		return -ENODEV;
 
 	/* If this file was open for write, then erase contents */
-	if ((file->f_mode & FMODE_WRITE) &&
-	    (file->f_flags & O_TRUNC)) {
-		if (tc->cpu == RING_BUFFER_ALL_CPUS)
+	if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) {
+		int cpu = tracing_get_cpu(inode);
+
+		if (cpu == RING_BUFFER_ALL_CPUS)
 			tracing_reset_online_cpus(&tr->trace_buffer);
 		else
-			tracing_reset(&tr->trace_buffer, tc->cpu);
+			tracing_reset(&tr->trace_buffer, cpu);
 	}
 
 	if (file->f_mode & FMODE_READ) {
-		iter = __tracing_open(tr, tc, inode, file, false);
+		iter = __tracing_open(inode, file, false);
 		if (IS_ERR(iter))
 			ret = PTR_ERR(iter);
 		else if (trace_flags & TRACE_ITER_LATENCY_FMT)
@@ -4676,8 +4670,7 @@ struct ftrace_buffer_info {
 #ifdef CONFIG_TRACER_SNAPSHOT
 static int tracing_snapshot_open(struct inode *inode, struct file *file)
 {
-	struct trace_cpu *tc = inode->i_private;
-	struct trace_array *tr = tc->tr;
+	struct trace_array *tr = inode->i_private;
 	struct trace_iterator *iter;
 	struct seq_file *m;
 	int ret = 0;
@@ -4686,7 +4679,7 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file)
 		return -ENODEV;
 
 	if (file->f_mode & FMODE_READ) {
-		iter = __tracing_open(tr, tc, inode, file, true);
+		iter = __tracing_open(inode, file, true);
 		if (IS_ERR(iter))
 			ret = PTR_ERR(iter);
 	} else {
@@ -4703,8 +4696,8 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file)
 		ret = 0;
 
 		iter->tr = tr;
-		iter->trace_buffer = &tc->tr->max_buffer;
-		iter->cpu_file = tc->cpu;
+		iter->trace_buffer = &tr->max_buffer;
+		iter->cpu_file = tracing_get_cpu(inode);
 		m->private = iter;
 		file->private_data = m;
 	}
@@ -5521,7 +5514,6 @@ trace_create_cpu_file(const char *name, umode_t mode, struct dentry *parent,
 static void
 tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 {
-	struct trace_array_cpu *data = per_cpu_ptr(tr->trace_buffer.data, cpu);
 	struct dentry *d_percpu = tracing_dentry_percpu(tr, cpu);
 	struct dentry *d_cpu;
 	char cpu_dir[30]; /* 30 characters should be more than enough */
@@ -5542,7 +5534,7 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 
 	/* per cpu trace */
 	trace_create_cpu_file("trace", 0644, d_cpu,
-				&data->trace_cpu, cpu, &tracing_fops);
+				tr, cpu, &tracing_fops);
 
 	trace_create_cpu_file("trace_pipe_raw", 0444, d_cpu,
 				tr, cpu, &tracing_buffers_fops);
@@ -5555,7 +5547,7 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
 
 #ifdef CONFIG_TRACER_SNAPSHOT
 	trace_create_cpu_file("snapshot", 0644, d_cpu,
-			  	&data->trace_cpu, cpu, &snapshot_fops);
+			  	tr, cpu, &snapshot_fops);
 
 	trace_create_cpu_file("snapshot_raw", 0444, d_cpu,
 				tr, cpu, &snapshot_raw_fops);
@@ -6121,7 +6113,7 @@ init_tracer_debugfs(struct trace_array *tr, struct dentry *d_tracer)
 			  tr, &tracing_iter_fops);
 
 	trace_create_file("trace", 0644, d_tracer,
-			(void *)&tr->trace_cpu, &tracing_fops);
+			  tr, &tracing_fops);
 
 	trace_create_file("trace_pipe", 0444, d_tracer,
 			  tr, &tracing_pipe_fops);
@@ -6142,11 +6134,11 @@ init_tracer_debugfs(struct trace_array *tr, struct dentry *d_tracer)
 			  &trace_clock_fops);
 
 	trace_create_file("tracing_on", 0644, d_tracer,
-			    tr, &rb_simple_fops);
+			  tr, &rb_simple_fops);
 
 #ifdef CONFIG_TRACER_SNAPSHOT
 	trace_create_file("snapshot", 0644, d_tracer,
-			  (void *)&tr->trace_cpu, &snapshot_fops);
+			  tr, &snapshot_fops);
 #endif
 
 	for_each_tracing_cpu(cpu)
-- 
1.5.5.1

[PATCH 7/7] tracing: Kill trace_cpu struct/members

[Oleg Nesterov]

After the previous changes trace_array_cpu->trace_cpu and
trace_array->trace_cpu becomes write-only. Remove these members
and kill "struct trace_cpu" as well.

As a side effect this also removes memset(per_cpu_memory, 0).
It was not needed, alloc_percpu() returns zero-filled memory.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace.c |   21 ---------------------
 kernel/trace/trace.h |    8 --------
 2 files changed, 0 insertions(+), 29 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0c0b4d5..9177780 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5859,17 +5859,6 @@ struct dentry *trace_instance_dir;
 static void
 init_tracer_debugfs(struct trace_array *tr, struct dentry *d_tracer);
 
-static void init_trace_buffers(struct trace_array *tr, struct trace_buffer *buf)
-{
-	int cpu;
-
-	for_each_tracing_cpu(cpu) {
-		memset(per_cpu_ptr(buf->data, cpu), 0, sizeof(struct trace_array_cpu));
-		per_cpu_ptr(buf->data, cpu)->trace_cpu.cpu = cpu;
-		per_cpu_ptr(buf->data, cpu)->trace_cpu.tr = tr;
-	}
-}
-
 static int
 allocate_trace_buffer(struct trace_array *tr, struct trace_buffer *buf, int size)
 {
@@ -5887,8 +5876,6 @@ allocate_trace_buffer(struct trace_array *tr, struct trace_buffer *buf, int size
 		return -ENOMEM;
 	}
 
-	init_trace_buffers(tr, buf);
-
 	/* Allocate the first page for all buffers */
 	set_buffer_entries(&tr->trace_buffer,
 			   ring_buffer_size(tr->trace_buffer.buffer, 0));
@@ -5955,10 +5942,6 @@ static int new_instance_create(const char *name)
 	if (allocate_trace_buffers(tr, trace_buf_size) < 0)
 		goto out_free_tr;
 
-	/* Holder for file callbacks */
-	tr->trace_cpu.cpu = RING_BUFFER_ALL_CPUS;
-	tr->trace_cpu.tr = tr;
-
 	tr->dir = debugfs_create_dir(name, trace_instance_dir);
 	if (!tr->dir)
 		goto out_free_tr;
@@ -6432,10 +6415,6 @@ __init static int tracer_alloc_buffers(void)
 
 	global_trace.flags = TRACE_ARRAY_FL_GLOBAL;
 
-	/* Holder for file callbacks */
-	global_trace.trace_cpu.cpu = RING_BUFFER_ALL_CPUS;
-	global_trace.trace_cpu.tr = &global_trace;
-
 	INIT_LIST_HEAD(&global_trace.systems);
 	INIT_LIST_HEAD(&global_trace.events);
 	list_add(&global_trace.list, &ftrace_trace_arrays);
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index e7d643b..afaae41 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -130,19 +130,12 @@ enum trace_flag_type {
 
 struct trace_array;
 
-struct trace_cpu {
-	struct trace_array	*tr;
-	struct dentry		*dir;
-	int			cpu;
-};
-
 /*
  * The CPU trace array - it consists of thousands of trace entries
  * plus some other descriptor data: (for example which task started
  * the trace, etc.)
  */
 struct trace_array_cpu {
-	struct trace_cpu	trace_cpu;
 	atomic_t		disabled;
 	void			*buffer_page;	/* ring buffer spare */
 
@@ -196,7 +189,6 @@ struct trace_array {
 	bool			allocated_snapshot;
 #endif
 	int			buffer_disabled;
-	struct trace_cpu	trace_cpu;	/* place holder */
 #ifdef CONFIG_FTRACE_SYSCALLS
 	int			sys_refcount_enter;
 	int			sys_refcount_exit;
-- 
1.5.5.1

Re: [PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Steven Rostedt]

Al,

I would like to get your opinion on this patch. Let me bring you up to
speed to what we are doing. Note, we are using the debugfs file system
here.

There was an incorrect assumption about the use of i_private. When a
file is created, the i_private would be set to some allocated object.
After the file is removed, the object would be deleted.

The problem is that there's a slight race where a process may have
opened the file, up the ref count of the dentry, and even though the
file was deleted, it's not really because it still has a reference
opened. When the object that i_private points to is freed, that leaves
the race where the process that has the file opened might still access
that object and crash.

Funny thing is, part of the use had this already covered. The
trace_array (tr) object has its own ref count. There's also a global
list of all trace_arrays. The get_trace_array(tr) will search for the tr
in the global list (under lock) and if it is found, it will up the ref
count. If not, it returns false and the "open()" call will fail. When
trace_array_get() succeeds, the ref count is incremented and any attempt
to delete it will fail. Thus, the trace_array (tr) part is not a
problem.

The problem is that some files only references the tr for a certain CPU.
Another data structure was created when the file was opened called
trace_cpu (tc), which maps a CPU and a trace_array. As there is no
global list of trace_cpus, this is susceptible to the above race
condition. Accessing tc->tr can happen after tc has been freed.

Now here's why I'm emailing you.  What Oleg is doing here is instead of
creating this extra trace_cpu structure, he's using the inode->i_cdev to
store the CPU information (he's wrapped this with helper functions so we
can use any inode structure). He sets inode->i_cdev to CPU+1 or to
RING_BUFFER_ALL_CPUS (when all CPU info is needed). This also means that
if we set i_cdev = 0, it can be known that the file is in the process of
being deleted, and we wont even have to search the global trace_array
list. This patch still does the search, but that could be eliminated in
the future.

What's your thoughts on this?

Thanks!

-- Steve


On Mon, 2013-07-22 at 15:43 +0200, Oleg Nesterov wrote:
> Every "file_operations" used by tracing_init_debugfs_percpu is buggy.
> f_op->open/etc does:
> 
> 	1. struct trace_cpu *tc = inode->i_private;
> 	   struct trace_array *tr = tc->tr;
> 
> 	2. trace_array_get(tr) or fail;
> 
> 	3. do_something(tc);
> 
> But tc (and tr) can be already freed before trace_array_get() is called.
> And it doesn't matter whether this file is per-cpu or it was created by
> init_tracer_debugfs(), free_percpu() or kfree() are equally bad.
> 
> Note that even 1. is not safe, the freed memory can be unmapped. But even
> if it was safe trace_array_get() can wrongly succeed if we also race with
> the next new_instance_create() which can re-allocate the same tr, or tc
> was overwritten and ->tr points to the valid tr. In this case 3. uses the
> freed/reused memory.
> 
> Add the new trivial helper, trace_create_cpu_file() which simply calls
> trace_create_file() and encodes "cpu" in "struct inode". Another helper,
> tracing_get_cpu() will be used to read cpu_nr-or-RING_BUFFER_ALL_CPUS.
> 
> The patch abuses ->i_cdev to encode the number, it is never used unless
> the file is S_ISCHR(). But we could use something else, say, i_bytes or
> even ->d_fsdata. In any case this hack is hidden inside these 2 helpers,
> it would be trivial to change them if needed.
> 
> This patch only changes tracing_init_debugfs_percpu() to use the new
> trace_create_cpu_file(), the next patches will change file_operations.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/trace/trace.c |   46 ++++++++++++++++++++++++++++++++--------------
>  1 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 3f24777..1e0fae9 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2843,6 +2843,13 @@ static int s_show(struct seq_file *m, void *v)
>  	return 0;
>  }
>  
> +static inline int tracing_get_cpu(struct inode *inode)
> +{
> +	if (inode->i_cdev) /* See trace_create_cpu_file() */
> +		return (long)inode->i_cdev - 1;
> +	return RING_BUFFER_ALL_CPUS;
> +}
> +
>  static const struct seq_operations tracer_seq_ops = {
>  	.start		= s_start,
>  	.next		= s_next,
> @@ -5529,6 +5536,17 @@ static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu)
>  	return tr->percpu_dir;
>  }
>  
> +static struct dentry *
> +trace_create_cpu_file(const char *name, umode_t mode, struct dentry *parent,
> +		      void *data, long cpu, const struct file_operations *fops)
> +{
> +	struct dentry *ret = trace_create_file(name, mode, parent, data, fops);
> +
> +	if (ret) /* See tracing_get_cpu() */
> +		ret->d_inode->i_cdev = (void*)(cpu + 1);
> +	return ret;
> +}
> +
>  static void
>  tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
>  {
> @@ -5548,28 +5566,28 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
>  	}
>  
>  	/* per cpu trace_pipe */
> -	trace_create_file("trace_pipe", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_pipe_fops);
> +	trace_create_cpu_file("trace_pipe", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_pipe_fops);
>  
>  	/* per cpu trace */
> -	trace_create_file("trace", 0644, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_fops);
> +	trace_create_cpu_file("trace", 0644, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_fops);
>  
> -	trace_create_file("trace_pipe_raw", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_buffers_fops);
> +	trace_create_cpu_file("trace_pipe_raw", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_buffers_fops);
>  
> -	trace_create_file("stats", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_stats_fops);
> +	trace_create_cpu_file("stats", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_stats_fops);
>  
> -	trace_create_file("buffer_size_kb", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_entries_fops);
> +	trace_create_cpu_file("buffer_size_kb", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_entries_fops);
>  
>  #ifdef CONFIG_TRACER_SNAPSHOT
> -	trace_create_file("snapshot", 0644, d_cpu,
> -			  (void *)&data->trace_cpu, &snapshot_fops);
> +	trace_create_cpu_file("snapshot", 0644, d_cpu,
> +			  	&data->trace_cpu, cpu, &snapshot_fops);
>  
> -	trace_create_file("snapshot_raw", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &snapshot_raw_fops);
> +	trace_create_cpu_file("snapshot_raw", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &snapshot_raw_fops);
>  #endif
>  }
>  

Re: [PATCH 0/7] tracing: Kill the buggy trace_cpu

[Steven Rostedt]

On Mon, 2013-07-22 at 15:43 +0200, Oleg Nesterov wrote:
> Hello.
> 
> Only slightly tested so far. I'll try to test more and report
> the results.
> 
> Meanwhile it would be nice if you can take a look and review.
> 
> 
> This series kills tracing_open_generic_tc/trace_cpu/etc.
> 
> trace_array_get(inode->i_private) is mostly fine, we do not
> dereference this pointer untill trace_array_get() succeeds.
> But trace_array_get(tc->tr) is wrong by the same reason why
> tracing_open_generic_file/etc are wrong, see 1/7.
> 
> And not only trace_cpu is buggy, we can simplify the code if
> we kill this member.
> 

Oleg,

So far I like the patch set. I just want to get Al's OK before we do a
hack with an inode structure item.

Thanks!

-- Steve

Re: [PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Steven Rostedt]

On Mon, 2013-07-22 at 10:55 -0400, Steven Rostedt wrote:

> The problem is that some files only references the tr for a certain CPU.
> Another data structure was created when the file was opened called
> trace_cpu (tc), which maps a CPU and a trace_array. As there is no

Correction, the trace_cpu is created when the file is created, not when
it was opened.

> global list of trace_cpus, this is susceptible to the above race
> condition. Accessing tc->tr can happen after tc has been freed.

Re: [PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Steven Rostedt]

On Mon, 2013-07-22 at 15:43 +0200, Oleg Nesterov wrote:

Checkpatch nits...

> ---
>  kernel/trace/trace.c |   46 ++++++++++++++++++++++++++++++++--------------
>  1 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 3f24777..1e0fae9 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2843,6 +2843,13 @@ static int s_show(struct seq_file *m, void *v)
>  	return 0;
>  }
>  
> +static inline int tracing_get_cpu(struct inode *inode)
> +{
> +	if (inode->i_cdev) /* See trace_create_cpu_file() */
> +		return (long)inode->i_cdev - 1;
> +	return RING_BUFFER_ALL_CPUS;
> +}
> +
>  static const struct seq_operations tracer_seq_ops = {
>  	.start		= s_start,
>  	.next		= s_next,
> @@ -5529,6 +5536,17 @@ static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu)
>  	return tr->percpu_dir;
>  }
>  
> +static struct dentry *
> +trace_create_cpu_file(const char *name, umode_t mode, struct dentry *parent,
> +		      void *data, long cpu, const struct file_operations *fops)
> +{
> +	struct dentry *ret = trace_create_file(name, mode, parent, data, fops);
> +
> +	if (ret) /* See tracing_get_cpu() */
> +		ret->d_inode->i_cdev = (void*)(cpu + 1);

 (void *)(cpu + 1)
      ^
    space needed


> +	return ret;
> +}
> +
>  static void
>  tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
>  {
> @@ -5548,28 +5566,28 @@ tracing_init_debugfs_percpu(struct trace_array *tr, long cpu)
>  	}
>  
>  	/* per cpu trace_pipe */
> -	trace_create_file("trace_pipe", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_pipe_fops);
> +	trace_create_cpu_file("trace_pipe", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_pipe_fops);
>  
>  	/* per cpu trace */
> -	trace_create_file("trace", 0644, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_fops);
> +	trace_create_cpu_file("trace", 0644, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_fops);
>  
> -	trace_create_file("trace_pipe_raw", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_buffers_fops);
> +	trace_create_cpu_file("trace_pipe_raw", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_buffers_fops);
>  
> -	trace_create_file("stats", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_stats_fops);
> +	trace_create_cpu_file("stats", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_stats_fops);
>  
> -	trace_create_file("buffer_size_kb", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &tracing_entries_fops);
> +	trace_create_cpu_file("buffer_size_kb", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &tracing_entries_fops);
>  
>  #ifdef CONFIG_TRACER_SNAPSHOT
> -	trace_create_file("snapshot", 0644, d_cpu,
> -			  (void *)&data->trace_cpu, &snapshot_fops);
> +	trace_create_cpu_file("snapshot", 0644, d_cpu,
> +			  	&data->trace_cpu, cpu, &snapshot_fops);

Whitespace damage above. There's spaces in between the tabs.

If no other changes are needed to this patchset (waiting on word from
Al), I'll fix this up. Otherwise, you might want to fix them if you need
to send a follow up patch set.

-- Steve


>  
> -	trace_create_file("snapshot_raw", 0444, d_cpu,
> -			(void *)&data->trace_cpu, &snapshot_raw_fops);
> +	trace_create_cpu_file("snapshot_raw", 0444, d_cpu,
> +				&data->trace_cpu, cpu, &snapshot_raw_fops);
>  #endif
>  }
>  

Re: [PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Oleg Nesterov]

On 07/22, Steven Rostedt wrote:
>
> Now here's why I'm emailing you.  What Oleg is doing here is instead of
> creating this extra trace_cpu structure, he's using the inode->i_cdev to
> store the CPU information (he's wrapped this with helper functions so we
> can use any inode structure). He sets inode->i_cdev to CPU+1 or to
> RING_BUFFER_ALL_CPUS (when all CPU info is needed).

This doesn't really matter, but RING_BUFFER_ALL_CPUS is encoded as NULL
so we do not need to change init_tracer_debugfs(). inode_init_always()
clears ->i_cdev.

Al, I will appreciate it if can ack/nack this hack.

I chose i_cdev because it shares the same union with i_pipe/bdev, this
(I hope) obviously means that vfs can never use this pointer unless it
checks S_ISCHR().

Oleg.

Re: [PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Oleg Nesterov]

On 07/22, Steven Rostedt wrote:
>
> Whitespace damage above. There's spaces in between the tabs.
>
> If no other changes are needed to this patchset (waiting on word from
> Al), I'll fix this up.

Thanks!

> Otherwise, you might want to fix them if you need
> to send a follow up patch set.

Yes, will do.

Oleg.

Re: [PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Steven Rostedt]

On Mon, 2013-07-22 at 19:14 +0200, Oleg Nesterov wrote:
> On 07/22, Steven Rostedt wrote:
> >
> > Now here's why I'm emailing you.  What Oleg is doing here is instead of
> > creating this extra trace_cpu structure, he's using the inode->i_cdev to
> > store the CPU information (he's wrapped this with helper functions so we
> > can use any inode structure). He sets inode->i_cdev to CPU+1 or to
> > RING_BUFFER_ALL_CPUS (when all CPU info is needed).
> 
> This doesn't really matter, but RING_BUFFER_ALL_CPUS is encoded as NULL
> so we do not need to change init_tracer_debugfs(). inode_init_always()
> clears ->i_cdev.

Yeah, I noticed that after sending the email. We could just set
i_private to NULL and get the same effect.

-- Steve

Re: [PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Steven Rostedt]

On Mon, 2013-07-22 at 19:14 +0200, Oleg Nesterov wrote:

> Al, I will appreciate it if can ack/nack this hack.
> 

Yes Al, please ack or nack this hack or at least get back as the fact
that we may sack the hack if we can't keep it intact and taken aback due
to the lack of keeping in track the quality of the hack which will get
us flack and we'll be attacked for our slack and have to pack the hack
cause it's whacked.

Thanks ;-)

-- Steve

Re: [PATCH 1/7] tracing: Introduce trace_create_cpu_file() and tracing_get_cpu()

[Steven Rostedt]

On Mon, 2013-07-22 at 19:15 +0200, Oleg Nesterov wrote:

> > Otherwise, you might want to fix them if you need
> > to send a follow up patch set.
> 
> Yes, will do.
> 

After applying your patches, my tests crashed the kernel. It's fixed
with the following update. I guess you should merge this into your next
version.

-- Steve

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 5e5ff44..db561b5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2984,7 +2984,7 @@ static int tracing_release(struct inode *inode, struct file *file)
 {
 	struct trace_array *tr = inode->i_private;
 	struct seq_file *m = file->private_data;
-	struct trace_iterator *iter = m->private;
+	struct trace_iterator *iter;
 	int cpu;
 
 	if (!(file->f_mode & FMODE_READ)) {
@@ -2992,6 +2992,9 @@ static int tracing_release(struct inode *inode, struct file *file)
 		return 0;
 	}
 
+	/* Writes do not use seq_file */
+	iter = m->private;
+
 	mutex_lock(&trace_types_lock);
 
 	for_each_tracing_cpu(cpu) {