[PATCH] trace-cmd: Fix segmentation fault in tracecmd_read_at() in specific use case

[PATCH] trace-cmd: Fix segmentation fault in tracecmd_read_at() in specific use case

[Tzvetomir Stoyanov (VMware)]

There is a segmentation fault in update_page_info() when the requested page
is not loaded, handle->cpu_data[cpu].page is NULL. The problematic flow starts
from tracecmd_read_at() API, when reading offset in the first page (less than 4K),
and this page is still not loaded yet. The problem can be observed randomly -
there is a sporadic KernelShark crash when loading a file, browsing and
zooming events.

https://bugzilla.kernel.org/show_bug.cgi?id=205165
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 6102eb3..da77418 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -1278,7 +1278,8 @@ tracecmd_read_at(struct tracecmd_input *handle, unsigned long long offset,
 	/* check to see if we have this page already */
 	for (cpu = 0; cpu < handle->cpus; cpu++) {
 		if (handle->cpu_data[cpu].offset == page_offset &&
-		    handle->cpu_data[cpu].file_size)
+		    handle->cpu_data[cpu].file_size &&
+		    handle->cpu_data[cpu].page)
 			break;
 	}
 
-- 
2.21.0

Re: [PATCH] trace-cmd: Fix segmentation fault in tracecmd_read_at() in specific use case

[Steven Rostedt]

On Fri, 11 Oct 2019 14:33:53 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> There is a segmentation fault in update_page_info() when the requested page
> is not loaded, handle->cpu_data[cpu].page is NULL. The problematic flow starts
> from tracecmd_read_at() API, when reading offset in the first page (less than 4K),
> and this page is still not loaded yet. The problem can be observed randomly -
> there is a sporadic KernelShark crash when loading a file, browsing and
> zooming events.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=205165
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 6102eb3..da77418 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -1278,7 +1278,8 @@ tracecmd_read_at(struct tracecmd_input *handle, unsigned long long offset,
>  	/* check to see if we have this page already */
>  	for (cpu = 0; cpu < handle->cpus; cpu++) {
>  		if (handle->cpu_data[cpu].offset == page_offset &&
> -		    handle->cpu_data[cpu].file_size)
> +		    handle->cpu_data[cpu].file_size &&
> +		    handle->cpu_data[cpu].page)
>  			break;
>  	}
>  

This does indeed look like a legit bug. But instead of checking here
for page not existing, since it's not part of the criteria for finding
the page (if the offset matches, we still want to break), lets do the
check below:

	if (cpu < handle->cpus && handle->cpu_data[cpu].page) {
		if (pcpu)
			*pcpu = cpu;
		return read_event(handle, offset, cpu);
	} else
		return find_and_read_event(handle, offset, pcpu);

-- Steve