tracing: Kill the buggy trace_cpu

PATCH Superseded Oleg Nesterov 2 participants 17 messages Jul 22, 2013 – Jul 23, 2013 Tracing

lore

Share & tools

lore

Oleg Nesterov

author

2013-07-22 13:48 UTC #0 lore raw

cover tracing: Kill the buggy trace_cpu

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(-)

Oleg Nesterov

author

2013-07-22 13:48 UTC #1 lore raw

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

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 <[email protected]>

---
 kernel/trace/trace.c |   46 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 32 insertions(+), 14 deletions(-)

kernel/trace/trace.c +32 ~~-14~~

index 3f24777..1e0fae9 100644
@@ -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
 }

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-07-22 13:49 UTC #11 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/trace/trace.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

kernel/trace/trace.c +4 -5

index 311581a..5a7fbc3 100644
@@ -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
 }

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-07-22 13:49 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/trace/trace.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

kernel/trace/trace.c +6 -7

index 5a7fbc3..d6429e5 100644
@@ -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);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-07-22 13:49 UTC #13 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/trace/trace.c |   49 ++++++++++++-------------------------------------
 1 files changed, 12 insertions(+), 37 deletions(-)

kernel/trace/trace.c +12 ~~-37~~

index d6429e5..611113d 100644
@@ -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);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-07-22 13:49 UTC #14 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/trace/trace.c |   52 +++++++++++++++++++++----------------------------
 1 files changed, 22 insertions(+), 30 deletions(-)

kernel/trace/trace.c +22 ~~-30~~

index 611113d..0c0b4d5 100644
@@ -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)

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-07-22 13:49 UTC #15 lore raw

PATCH 7/7 tracing: Kill trace_cpu struct/members

In reply to 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 <[email protected]>

---
 kernel/trace/trace.c |   21 ---------------------
 kernel/trace/trace.h |    8 --------
 2 files changed, 0 insertions(+), 29 deletions(-)

kernel/trace/trace.c +0 ~~-21~~

index 0c0b4d5..9177780 100644
@@ -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);

kernel/trace/trace.h +0 -8

index e7d643b..afaae41 100644
@@ -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;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-07-22 13:50 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/trace/trace.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

kernel/trace/trace.c +7 -9

index 1e0fae9..311581a 100644
@@ -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);

Signature · 1 line

1.5.5.1

Steven Rostedt

maintainer

2013-07-22 14:56 UTC #2 lore raw

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

In reply to Oleg Nesterov

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

Quoted 116 lines · 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 <[email protected]>
---
 kernel/trace/trace.c |   46 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 32 insertions(+), 14 deletions(-)
kernel/trace/trace.c +32 ~~-14~~
index 3f24777..1e0fae9 100644
@@ -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
 }
 

Steven Rostedt

maintainer

2013-07-22 14:59 UTC #16 lore raw

In reply to Oleg Nesterov

Quoted 10 lines · 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.

Quoted 8 lines

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

Steven Rostedt

maintainer

2013-07-22 15:06 UTC #3 lore raw

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

In reply to Steven Rostedt

Quoted 3 lines · Steven Rostedt

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.

Quoted 2 lines

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

Steven Rostedt

maintainer

2013-07-22 15:23 UTC #7 lore raw

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

In reply to Oleg Nesterov

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

Checkpatch nits...

Quoted 24 lines

---
 kernel/trace/trace.c |   46 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 32 insertions(+), 14 deletions(-)

kernel/trace/trace.c +7 -0

index 3f24777..1e0fae9 100644
@@ -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;

kernel/trace/trace.c static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu) · line 5546

Quoted 10 lines

kernel/trace/trace.c +8 -0

 }
 
+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

Quoted 31 lines

kernel/trace/trace.c +11 -8

+	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);

kernel/trace/trace.c tracing_init_debugfs_percpu(struct trace_array *tr, long cpu) · line 5587

Quoted 10 lines

kernel/trace/trace.c +4 -4

-	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

Quoted 8 lines

kernel/trace/trace.c +2 -2

-	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
 }

Oleg Nesterov

author

2013-07-22 17:19 UTC #4 lore raw

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

In reply to Steven Rostedt

Quoted 6 lines · Steven Rostedt

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.

Oleg Nesterov

author

2013-07-22 17:21 UTC #8 lore raw

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

In reply to Steven Rostedt

Quoted 5 lines · Steven Rostedt

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!

Quoted 2 lines

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

Yes, will do.

Oleg.

Steven Rostedt

maintainer

2013-07-22 17:34 UTC #5 lore raw

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

In reply to Oleg Nesterov

Quoted 11 lines · Oleg Nesterov

Quoted 6 lines · Steven Rostedt
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

Steven Rostedt

maintainer

2013-07-22 17:46 UTC #6 lore raw

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

In reply to Oleg Nesterov

Quoted 2 lines · Oleg Nesterov

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

Steven Rostedt

maintainer

2013-07-23 00:44 UTC #9 lore raw

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

In reply to Oleg Nesterov

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines
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

kernel/trace/trace.c +4 -1

index 5e5ff44..db561b5 100644
@@ -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) {