Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1702009 > unrolled thread

Re: [PATCH] tracing: Fix trace_pipe_raw read panic

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH] tracing: Fix trace_pipe_raw read panic Chunyu Hu <chuhu.ncepu@gmail.com> - 2017-08-02 13:10 +0200
    Re: [PATCH] tracing: Fix trace_pipe_raw read panic Steven Rostedt <rostedt@goodmis.org> - 2017-08-02 20:20 +0200

#1702009 — Re: [PATCH] tracing: Fix trace_pipe_raw read panic

Thanks for the comments and review!

On 2 August 2017 at 00:22, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Hmm, I applied this without taking too much of a look at it. I'm going
> to modified it a little (sorry, I'm about to take another trip and need
> to get this tested before I leave).
>
> Comments below...
>
>
> On Mon, 24 Jul 2017 15:21:06 +0800
> Chunyu Hu <chuhu@redhat.com> wrote:
>
>> per_cpu trace directories and files are created for all possible cpus,
>> but only the cpus which have ever been on-lined have their own per cpu
>> ring buffer (allocated by cpuhp threads). While trace_buffers_open, the
>> open handler for trace file 'trace_pipe_raw' is always trying to access
>> field of ring_buffer_per_cpu, and would panic with the NULL pointer.
>>
>> Align the behavior of trace_pipe_raw with trace_pipe, that returns -NODEV
>> when openning it if that cpu does not have trace ring buffer.
>>
>> Reproduce:
>> cat /sys/kernel/debug/tracing/per_cpu/cpu31/trace_pipe_raw
>> (cpu31 is never on-lined, this is a 16 cores x86_64 box)
>>
>> Tested with:
>> 1) boot with maxcpus=14, read trace_pipe_raw of cpu15.
>>    Got -NODEV.
>> 2) oneline cpu15, read trace_pipe_raw of cpu15.
>>    Get the raw trace data.
>>
>> Call trace:
>> [ 5760.950995] RIP: 0010:ring_buffer_alloc_read_page+0x32/0xe0
>> [ 5760.961678]  tracing_buffers_read+0x1f6/0x230
>> [ 5760.962695]  __vfs_read+0x37/0x160
>> [ 5760.963498]  ? __vfs_read+0x5/0x160
>> [ 5760.964339]  ? security_file_permission+0x9d/0xc0
>> [ 5760.965451]  ? __vfs_read+0x5/0x160
>> [ 5760.966280]  vfs_read+0x8c/0x130
>> [ 5760.967070]  SyS_read+0x55/0xc0
>> [ 5760.967779]  do_syscall_64+0x67/0x150
>> [ 5760.968687]  entry_SYSCALL64_slow_path+0x25/0x25
>>
>> Signed-off-by: Chunyu Hu <chuhu@redhat.com>
>> ---
>>  kernel/trace/ring_buffer.c |  5 +++++
>>  kernel/trace/trace.c       | 14 +++++++++++++-
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
>> index 4ae268e..66c109e 100644
>> --- a/kernel/trace/ring_buffer.c
>> +++ b/kernel/trace/ring_buffer.c
>> @@ -3002,6 +3002,11 @@ int ring_buffer_write(struct ring_buffer *buffer,
>>  }
>>  EXPORT_SYMBOL_GPL(ring_buffer_write);
>>
>> +bool rb_per_cpu_allocated(struct ring_buffer *buffer, int cpu)
>> +{
>> +     return !!cpumask_test_cpu(cpu, buffer->cpumask);
>
> The "!!" is not needed. The compiler will convert it for you.

OK. After researching, agreed.

>
>> +}
>> +
>>  static bool rb_per_cpu_empty(struct ring_buffer_per_cpu *cpu_buffer)
>>  {
>>       struct buffer_page *reader = cpu_buffer->reader_page;
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 42b9355..508a1ca 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -6542,11 +6542,16 @@ static int snapshot_raw_open(struct inode *inode, struct file *filp)
>>
>>  #endif /* CONFIG_TRACER_SNAPSHOT */
>>
>> +extern bool rb_per_cpu_allocated(struct ring_buffer *buffer, int cpu);
>> +
>
> This must be in a header file.

OK. I hope that won't change symver.

>
>
>>  static int tracing_buffers_open(struct inode *inode, struct file *filp)
>>  {
>>       struct trace_array *tr = inode->i_private;
>> +     struct trace_buffer *tb = &tr->trace_buffer;
>
> Other places use the variable "trace_buf" for this. Need to stay
> consistent.

Agreed.


>
>> +     struct ring_buffer *buffer = tb->buffer;
>
> Actually, why even have that variable? It's not used anywhere else.

Agreed.

>
>>       struct ftrace_buffer_info *info;
>>       int ret;
>> +     int cpu_file;
>>
>>       if (tracing_disabled)
>>               return -ENODEV;
>> @@ -6554,6 +6559,13 @@ static int tracing_buffers_open(struct inode *inode, struct file *filp)
>>       if (trace_array_get(tr) < 0)
>>               return -ENODEV;
>
> Not to mention, the tr can not be dereferenced until after the above
> get.

Ah, my bad. I missed this.

>
> I'll make the changes, and I'll keep you as author.

Thanks.

>
> Thanks!
>
> -- Steve
>
>>
>> +     cpu_file = tracing_get_cpu(inode);
>> +
>> +     /* Make sure, ring buffer for this cpu is allocated. */
>> +     if (cpu_file != RING_BUFFER_ALL_CPUS &&
>> +                     !rb_per_cpu_allocated(buffer, cpu_file))
>> +             return -ENODEV;
>> +
>>       info = kzalloc(sizeof(*info), GFP_KERNEL);
>>       if (!info) {
>>               trace_array_put(tr);
>> @@ -6563,7 +6575,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     = tracing_get_cpu(inode);
>> +     info->iter.cpu_file     = cpu_file;
>>       info->iter.trace        = tr->current_trace;
>>       info->iter.trace_buffer = &tr->trace_buffer;
>>       info->spare             = NULL;
>

[toc] | [next] | [standalone]

#1702359

On Wed, 2 Aug 2017 19:03:50 +0800
Chunyu Hu <chuhu.ncepu@gmail.com> wrote:

> Thanks for the comments and review!
> 

OK, I did a bisect to see where this bug happened, and discovered that
it's the creation of the allocated reader pages. I don't like this
solution because it places the work to know what buffers are available
to the users of the ring buffer. The ring buffer should be robust
enough to handle it itself.

I wrote up this patch instead, and I think this is the better solution.
As it doesn't require the users of the ring buffer to know what CPUs
are allocated or not.

-- Steve

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 529cc50..81279c6 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -4386,15 +4386,19 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
  * the page that was allocated, with the read page of the buffer.
  *
  * Returns:
- *  The page allocated, or NULL on error.
+ *  The page allocated, or ERR_PTR
  */
 void *ring_buffer_alloc_read_page(struct ring_buffer *buffer, int cpu)
 {
-	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
+	struct ring_buffer_per_cpu *cpu_buffer;
 	struct buffer_data_page *bpage = NULL;
 	unsigned long flags;
 	struct page *page;
 
+	if (!cpumask_test_cpu(cpu, buffer->cpumask))
+		return ERR_PTR(-ENODEV);
+
+	cpu_buffer = buffer->buffers[cpu];
 	local_irq_save(flags);
 	arch_spin_lock(&cpu_buffer->lock);
 
@@ -4412,7 +4416,7 @@ void *ring_buffer_alloc_read_page(struct ring_buffer *buffer, int cpu)
 	page = alloc_pages_node(cpu_to_node(cpu),
 				GFP_KERNEL | __GFP_NORETRY, 0);
 	if (!page)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	bpage = page_address(page);
 
@@ -4467,8 +4471,8 @@ EXPORT_SYMBOL_GPL(ring_buffer_free_read_page);
  *
  * for example:
  *	rpage = ring_buffer_alloc_read_page(buffer, cpu);
- *	if (!rpage)
- *		return error;
+ *	if (IS_ERR(rpage))
+ *		return PTR_ERR(rpage);
  *	ret = ring_buffer_read_page(buffer, &rpage, len, cpu, 0);
  *	if (ret >= 0)
  *		process_page(rpage, ret);
diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index 9fbcaf5..68ee79a 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -113,7 +113,7 @@ static enum event_status read_page(int cpu)
 	int i;
 
 	bpage = ring_buffer_alloc_read_page(buffer, cpu);
-	if (!bpage)
+	if (IS_ERR(bpage))
 		return EVENT_DROPPED;
 
 	ret = ring_buffer_read_page(buffer, &bpage, PAGE_SIZE, cpu, 1);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 42b9355..3edba2f 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6598,7 +6598,7 @@ tracing_buffers_read(struct file *filp, char __user *ubuf,
 {
 	struct ftrace_buffer_info *info = filp->private_data;
 	struct trace_iterator *iter = &info->iter;
-	ssize_t ret;
+	ssize_t ret = 0;
 	ssize_t size;
 
 	if (!count)
@@ -6612,10 +6612,15 @@ tracing_buffers_read(struct file *filp, char __user *ubuf,
 	if (!info->spare) {
 		info->spare = ring_buffer_alloc_read_page(iter->trace_buffer->buffer,
 							  iter->cpu_file);
-		info->spare_cpu = iter->cpu_file;
+		if (IS_ERR(info->spare)) {
+			ret = PTR_ERR(info->spare);
+			info->spare = NULL;
+		} else {
+			info->spare_cpu = iter->cpu_file;
+		}
 	}
 	if (!info->spare)
-		return -ENOMEM;
+		return ret;
 
 	/* Do we have previous read data to read? */
 	if (info->read < PAGE_SIZE)
@@ -6790,8 +6795,9 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 		ref->ref = 1;
 		ref->buffer = iter->trace_buffer->buffer;
 		ref->page = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file);
-		if (!ref->page) {
-			ret = -ENOMEM;
+		if (IS_ERR(ref->page)) {
+			ret = PTR_ERR(ref->page);
+			ref->page = NULL;
 			kfree(ref);
 			break;
 		}

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web