[PATCH] tracing: Fix trace_pipe_raw read panic

[PATCH] tracing: Fix trace_pipe_raw read panic

[Chunyu Hu]

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);
+}
+
 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);
+
 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;
+	struct ring_buffer *buffer = tb->buffer;
 	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;
 
+	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;
-- 
1.8.3.1

Re: [PATCH] tracing: Fix trace_pipe_raw read panic

[Steven Rostedt]

Hi Chunyu,

Thanks for the patch. I'm currently traveling, and will have little
time to test it. Hopefully I can get to it sometime this week.

-- Steve


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.

Re: [PATCH] tracing: Fix trace_pipe_raw read panic

[Chunyu Hu]

A gentle ping. Any comment on this? Maybe have better solution for this?

On 25 July 2017 at 04:24, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Hi Chunyu,
>
> Thanks for the patch. I'm currently traveling, and will have little
> time to test it. Hopefully I can get to it sometime this week.
>
> -- Steve
>
>
> 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.

Re: [PATCH] tracing: Fix trace_pipe_raw read panic

[Steven Rostedt]

On Tue, 1 Aug 2017 21:02:12 +0800
Chunyu Hu <chuhu.ncepu@gmail.com> wrote:

> A gentle ping. Any comment on this? Maybe have better solution for this?

I actually applied this yesterday. Unfortunately, as I applied it to
rc3, a new bug in mainline turned up triggering my test suite to fail.

I'm currently looking at why my tests failed (it had nothing to do with
this patch). Consider this patch already pulled, as it is in the queue
to go to Linus (I pushed it to my ftrace/core branch in my repo too).

-- Steve

Re: [PATCH] tracing: Fix trace_pipe_raw read panic

[Steven Rostedt]

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.

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


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

> +	struct ring_buffer *buffer = tb->buffer;

Actually, why even have that variable? It's not used anywhere else.

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

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

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;

Re: [PATCH] tracing: Fix trace_pipe_raw read panic

[Steven Rostedt]

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

Re: [PATCH] tracing: Fix trace_pipe_raw read panic

[Chunyu Hu]

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

Re: [PATCH] tracing: Fix trace_pipe_raw read panic

[Chunyu Hu]

On 24 July 2017 at 03:21, 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);
> +}
> +
>  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);
> +
>  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;
> +       struct ring_buffer *buffer = tb->buffer;
>         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;
>
> +       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))

oops, here miss a  trace_array_put.

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