linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] kernel-shark: Add check for return of ksmodel_get_cpu_front()
@ 2021-05-07 13:38 Steven Rostedt
  2021-05-11 13:25 ` Yordan Karadzhov
  2021-05-14 11:50 ` Yordan Karadzhov
  0 siblings, 2 replies; 5+ messages in thread
From: Steven Rostedt @ 2021-05-07 13:38 UTC (permalink / raw)
  To: Yordan Karadzhov; +Cc: Linux Trace Devel

From: Steven Rostedt (VMware) <rostedt@goodmis.org>

When I loaded two trace.dat files (host and guest), but the mapping of
which host thread is associated to which guest vCPU was missing from the
file, it caused a SEGFAULT. That's because in fillTaskGraph(), the
lamGetPidCPU() calls ksmodule_get_cpu_front() which returns a negative
number and does not set index. But the next line checks data[index] where
index is some random number, and the application crashes.

By checking the return of ksmodule_get_cpu_front(), and if it is negative
do not reference data[index] and just let eFront be nullptr, the
application shows no mapping, but at least it does not crash.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
You can reproduce this with:

   http://rostedt.org/private/trace-no-map.tar.bz2

diff --git a/src/KsPlotTools.cpp b/src/KsPlotTools.cpp
index 225dc34..abef5f8 100644
--- a/src/KsPlotTools.cpp
+++ b/src/KsPlotTools.cpp
@@ -1280,7 +1280,7 @@ void Graph::fillTaskGraph(int sd, int pid)
 						 false,
 						 _collectionPtr,
 						 &index);
-		if (index >= 0)
+		if (cpuFront >= 0 && index >= 0)
 			eFront = _histoPtr->data[index];
 
 		cpuBack = ksmodel_get_cpu_back(_histoPtr, bin,

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] kernel-shark: Add check for return of ksmodel_get_cpu_front()
  2021-05-07 13:38 [PATCH] kernel-shark: Add check for return of ksmodel_get_cpu_front() Steven Rostedt
@ 2021-05-11 13:25 ` Yordan Karadzhov
  2021-05-11 13:40   ` Steven Rostedt
  2021-05-14 11:50 ` Yordan Karadzhov
  1 sibling, 1 reply; 5+ messages in thread
From: Yordan Karadzhov @ 2021-05-11 13:25 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Linux Trace Devel

Hi Steven,

On 7.05.21 г. 16:38, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> When I loaded two trace.dat files (host and guest), but the mapping of
> which host thread is associated to which guest vCPU was missing from the
> file, it caused a SEGFAULT. That's because in fillTaskGraph(), the
> lamGetPidCPU() calls ksmodule_get_cpu_front() which returns a negative
> number and does not set index. But the next line checks data[index] where
> index is some random number, and the application crashes.
> 
> By checking the return of ksmodule_get_cpu_front(), and if it is negative
> do not reference data[index] and just let eFront be nullptr, the
> application shows no mapping, but at least it does not crash.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> You can reproduce this with:
> 
>     http://rostedt.org/private/trace-no-map.tar.bz2
> 
> diff --git a/src/KsPlotTools.cpp b/src/KsPlotTools.cpp
> index 225dc34..abef5f8 100644
> --- a/src/KsPlotTools.cpp
> +++ b/src/KsPlotTools.cpp
> @@ -1280,7 +1280,7 @@ void Graph::fillTaskGraph(int sd, int pid)
>   						 false,
>   						 _collectionPtr,
>   						 &index);

I wonder why this fails? Is it because "pid" is negative? The fix below 
is appropriate, however we should detect negative PIDs and abort 
plotting much earlier.

Thanks!
Yordan

> -		if (index >= 0)
> +		if (cpuFront >= 0 && index >= 0)
>   			eFront = _histoPtr->data[index];
>   
>   		cpuBack = ksmodel_get_cpu_back(_histoPtr, bin,
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] kernel-shark: Add check for return of ksmodel_get_cpu_front()
  2021-05-11 13:25 ` Yordan Karadzhov
@ 2021-05-11 13:40   ` Steven Rostedt
  2021-05-11 13:51     ` Yordan Karadzhov
  0 siblings, 1 reply; 5+ messages in thread
From: Steven Rostedt @ 2021-05-11 13:40 UTC (permalink / raw)
  To: Yordan Karadzhov; +Cc: Linux Trace Devel

On Tue, 11 May 2021 16:25:35 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> > You can reproduce this with:
> > 
> >     http://rostedt.org/private/trace-no-map.tar.bz2
> > 
> > diff --git a/src/KsPlotTools.cpp b/src/KsPlotTools.cpp
> > index 225dc34..abef5f8 100644
> > --- a/src/KsPlotTools.cpp
> > +++ b/src/KsPlotTools.cpp
> > @@ -1280,7 +1280,7 @@ void Graph::fillTaskGraph(int sd, int pid)
> >   						 false,
> >   						 _collectionPtr,
> >   						 &index);  
> 
> I wonder why this fails? Is it because "pid" is negative? The fix below 
> is appropriate, however we should detect negative PIDs and abort 
> plotting much earlier.

I believe the issue is that we failed to map which host task goes with
which guest vCPUU, and just randomly picked one (or none).

I fixed trace-cmd to get the mappings when qemu is not found, and
KernelShark works fine on that case. This bug only appears when it can't
find the host thread that corresponds to the guest vCPU.

-- Steve

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] kernel-shark: Add check for return of ksmodel_get_cpu_front()
  2021-05-11 13:40   ` Steven Rostedt
@ 2021-05-11 13:51     ` Yordan Karadzhov
  0 siblings, 0 replies; 5+ messages in thread
From: Yordan Karadzhov @ 2021-05-11 13:51 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Linux Trace Devel



On 11.05.21 г. 16:40, Steven Rostedt wrote:
> On Tue, 11 May 2021 16:25:35 +0300
> Yordan Karadzhov <y.karadz@gmail.com> wrote:
> 
>>> You can reproduce this with:
>>>
>>>      http://rostedt.org/private/trace-no-map.tar.bz2

What should I do to make it SEGFAULT?
Y.

>>>
>>> diff --git a/src/KsPlotTools.cpp b/src/KsPlotTools.cpp
>>> index 225dc34..abef5f8 100644
>>> --- a/src/KsPlotTools.cpp
>>> +++ b/src/KsPlotTools.cpp
>>> @@ -1280,7 +1280,7 @@ void Graph::fillTaskGraph(int sd, int pid)
>>>    						 false,
>>>    						 _collectionPtr,
>>>    						 &index);
>>
>> I wonder why this fails? Is it because "pid" is negative? The fix below
>> is appropriate, however we should detect negative PIDs and abort
>> plotting much earlier.
> 
> I believe the issue is that we failed to map which host task goes with
> which guest vCPUU, and just randomly picked one (or none).
> 
> I fixed trace-cmd to get the mappings when qemu is not found, and
> KernelShark works fine on that case. This bug only appears when it can't
> find the host thread that corresponds to the guest vCPU.
> 
> -- Steve
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] kernel-shark: Add check for return of ksmodel_get_cpu_front()
  2021-05-07 13:38 [PATCH] kernel-shark: Add check for return of ksmodel_get_cpu_front() Steven Rostedt
  2021-05-11 13:25 ` Yordan Karadzhov
@ 2021-05-14 11:50 ` Yordan Karadzhov
  1 sibling, 0 replies; 5+ messages in thread
From: Yordan Karadzhov @ 2021-05-14 11:50 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Linux Trace Devel



On 7.05.21 г. 16:38, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> When I loaded two trace.dat files (host and guest), but the mapping of
> which host thread is associated to which guest vCPU was missing from the
> file, it caused a SEGFAULT. That's because in fillTaskGraph(), the
> lamGetPidCPU() calls ksmodule_get_cpu_front() which returns a negative
> number and does not set index. But the next line checks data[index] where
> index is some random number, and the application crashes.
> 
> By checking the return of ksmodule_get_cpu_front(), and if it is negative
> do not reference data[index] and just let eFront be nullptr, the
> application shows no mapping, but at least it does not crash.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>


I am applying this one.

Thanks Steven!
Y.

> ---
> You can reproduce this with:
> 
>     http://rostedt.org/private/trace-no-map.tar.bz2
> 
> diff --git a/src/KsPlotTools.cpp b/src/KsPlotTools.cpp
> index 225dc34..abef5f8 100644
> --- a/src/KsPlotTools.cpp
> +++ b/src/KsPlotTools.cpp
> @@ -1280,7 +1280,7 @@ void Graph::fillTaskGraph(int sd, int pid)
>   						 false,
>   						 _collectionPtr,
>   						 &index);
> -		if (index >= 0)
> +		if (cpuFront >= 0 && index >= 0)
>   			eFront = _histoPtr->data[index];
>   
>   		cpuBack = ksmodel_get_cpu_back(_histoPtr, bin,
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-05-14 11:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07 13:38 [PATCH] kernel-shark: Add check for return of ksmodel_get_cpu_front() Steven Rostedt
2021-05-11 13:25 ` Yordan Karadzhov
2021-05-11 13:40   ` Steven Rostedt
2021-05-11 13:51     ` Yordan Karadzhov
2021-05-14 11:50 ` Yordan Karadzhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).