linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Copy name string into ring buffer for intel_update/disable_plane tracepoints
@ 2019-07-10 16:01 Steven Rostedt
  2019-07-10 17:12 ` [PATCH v2] " Ville Syrjala
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2019-07-10 16:01 UTC (permalink / raw)
  To: intel-gfx, dri-devel, LKML
  Cc: Ville Syrjälä, Maarten Lankhorst, David Airlie, Daniel Vetter


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

Currently the intel_update_plane and intel_disable_plane tracepoints record
the address of plane->name in the ring buffer, and then when reading the
ring buffer uses %s to get the name. The issue with this, is that those two
events can be minutes, hours or even days apart. It is very dangerous to
dereference a string pointer without knowing if it still exists or not.

The proper way to handle this is to use the __string() macro in the
tracepoint which will save the string into the ring buffer at the time of
recording. Then there's no worries if the original string still exists in
memory when the ring buffer is read.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 drivers/gpu/drm/i915/i915_trace.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 12893304c8f8..d41d914a16ca 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -298,16 +298,16 @@ TRACE_EVENT(intel_update_plane,
 
 	    TP_STRUCT__entry(
 			     __field(enum pipe, pipe)
-			     __field(const char *, name)
 			     __field(u32, frame)
 			     __field(u32, scanline)
 			     __array(int, src, 4)
 			     __array(int, dst, 4)
+			     __string(name, plane->name)
 			     ),
 
 	    TP_fast_assign(
+			   __assign_str(name, plane->name);
 			   __entry->pipe = crtc->pipe;
-			   __entry->name = plane->name;
 			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
 										       crtc->pipe);
 			   __entry->scanline = intel_get_crtc_scanline(crtc);
@@ -316,7 +316,7 @@ TRACE_EVENT(intel_update_plane,
 			   ),
 
 	    TP_printk("pipe %c, plane %s, frame=%u, scanline=%u, " DRM_RECT_FP_FMT " -> " DRM_RECT_FMT,
-		      pipe_name(__entry->pipe), __entry->name,
+		      pipe_name(__entry->pipe), __get_str(name),
 		      __entry->frame, __entry->scanline,
 		      DRM_RECT_FP_ARG((const struct drm_rect *)__entry->src),
 		      DRM_RECT_ARG((const struct drm_rect *)__entry->dst))
@@ -328,21 +328,21 @@ TRACE_EVENT(intel_disable_plane,
 
 	    TP_STRUCT__entry(
 			     __field(enum pipe, pipe)
-			     __field(const char *, name)
 			     __field(u32, frame)
 			     __field(u32, scanline)
+			     __string(name, plane->name)
 			     ),
 
 	    TP_fast_assign(
+			   __assign_str(name, plane->name);
 			   __entry->pipe = crtc->pipe;
-			   __entry->name = plane->name;
 			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
 										       crtc->pipe);
 			   __entry->scanline = intel_get_crtc_scanline(crtc);
 			   ),
 
 	    TP_printk("pipe %c, plane %s, frame=%u, scanline=%u",
-		      pipe_name(__entry->pipe), __entry->name,
+		      pipe_name(__entry->pipe), __get_str(name),
 		      __entry->frame, __entry->scanline)
 );
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* [PATCH v2] drm/i915: Copy name string into ring buffer for intel_update/disable_plane tracepoints
  2019-07-10 16:01 [PATCH] drm/i915: Copy name string into ring buffer for intel_update/disable_plane tracepoints Steven Rostedt
@ 2019-07-10 17:12 ` Ville Syrjala
  2019-07-11 19:55   ` Ville Syrjälä
  0 siblings, 1 reply; 3+ messages in thread
From: Ville Syrjala @ 2019-07-10 17:12 UTC (permalink / raw)
  To: intel-gfx; +Cc: dri-devel, linux-kernel, Steven Rostedt (VMware)

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

Currently the intel_update_plane and intel_disable_plane tracepoints record
the address of plane->name in the ring buffer, and then when reading the
ring buffer uses %s to get the name. The issue with this, is that those two
events can be minutes, hours or even days apart. It is very dangerous to
dereference a string pointer without knowing if it still exists or not.

The proper way to handle this is to use the __string() macro in the
tracepoint which will save the string into the ring buffer at the time of
recording. Then there's no worries if the original string still exists in
memory when the ring buffer is read.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
[vsyrjala: Rebase on top of drm-tip]
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_trace.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index cce426b23a24..da18b8d6b80c 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -293,16 +293,16 @@ TRACE_EVENT(intel_update_plane,
 
 	    TP_STRUCT__entry(
 			     __field(enum pipe, pipe)
-			     __field(const char *, name)
 			     __field(u32, frame)
 			     __field(u32, scanline)
 			     __array(int, src, 4)
 			     __array(int, dst, 4)
+			     __string(name, plane->name)
 			     ),
 
 	    TP_fast_assign(
+			   __assign_str(name, plane->name);
 			   __entry->pipe = crtc->pipe;
-			   __entry->name = plane->name;
 			   __entry->frame = intel_crtc_get_vblank_counter(crtc);
 			   __entry->scanline = intel_get_crtc_scanline(crtc);
 			   memcpy(__entry->src, &plane->state->src, sizeof(__entry->src));
@@ -310,7 +310,7 @@ TRACE_EVENT(intel_update_plane,
 			   ),
 
 	    TP_printk("pipe %c, plane %s, frame=%u, scanline=%u, " DRM_RECT_FP_FMT " -> " DRM_RECT_FMT,
-		      pipe_name(__entry->pipe), __entry->name,
+		      pipe_name(__entry->pipe), __get_str(name),
 		      __entry->frame, __entry->scanline,
 		      DRM_RECT_FP_ARG((const struct drm_rect *)__entry->src),
 		      DRM_RECT_ARG((const struct drm_rect *)__entry->dst))
@@ -322,20 +322,20 @@ TRACE_EVENT(intel_disable_plane,
 
 	    TP_STRUCT__entry(
 			     __field(enum pipe, pipe)
-			     __field(const char *, name)
 			     __field(u32, frame)
 			     __field(u32, scanline)
+			     __string(name, plane->name)
 			     ),
 
 	    TP_fast_assign(
+			   __assign_str(name, plane->name);
 			   __entry->pipe = crtc->pipe;
-			   __entry->name = plane->name;
 			   __entry->frame = intel_crtc_get_vblank_counter(crtc);
 			   __entry->scanline = intel_get_crtc_scanline(crtc);
 			   ),
 
 	    TP_printk("pipe %c, plane %s, frame=%u, scanline=%u",
-		      pipe_name(__entry->pipe), __entry->name,
+		      pipe_name(__entry->pipe), __get_str(name),
 		      __entry->frame, __entry->scanline)
 );
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/i915: Copy name string into ring buffer for intel_update/disable_plane tracepoints
  2019-07-10 17:12 ` [PATCH v2] " Ville Syrjala
@ 2019-07-11 19:55   ` Ville Syrjälä
  0 siblings, 0 replies; 3+ messages in thread
From: Ville Syrjälä @ 2019-07-11 19:55 UTC (permalink / raw)
  To: intel-gfx; +Cc: dri-devel, linux-kernel, Steven Rostedt (VMware)

On Wed, Jul 10, 2019 at 08:12:30PM +0300, Ville Syrjala wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Currently the intel_update_plane and intel_disable_plane tracepoints record
> the address of plane->name in the ring buffer, and then when reading the
> ring buffer uses %s to get the name. The issue with this, is that those two
> events can be minutes, hours or even days apart. It is very dangerous to
> dereference a string pointer without knowing if it still exists or not.
> 
> The proper way to handle this is to use the __string() macro in the
> tracepoint which will save the string into the ring buffer at the time of
> recording. Then there's no worries if the original string still exists in
> memory when the ring buffer is read.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> [vsyrjala: Rebase on top of drm-tip]
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

CI is happy (not that we test this stuff) and I'm happy (the tracepoints
still work) -> pushed to drm-intel-next-queued. Thanks for the patch.

> ---
>  drivers/gpu/drm/i915/i915_trace.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index cce426b23a24..da18b8d6b80c 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -293,16 +293,16 @@ TRACE_EVENT(intel_update_plane,
>  
>  	    TP_STRUCT__entry(
>  			     __field(enum pipe, pipe)
> -			     __field(const char *, name)
>  			     __field(u32, frame)
>  			     __field(u32, scanline)
>  			     __array(int, src, 4)
>  			     __array(int, dst, 4)
> +			     __string(name, plane->name)
>  			     ),
>  
>  	    TP_fast_assign(
> +			   __assign_str(name, plane->name);
>  			   __entry->pipe = crtc->pipe;
> -			   __entry->name = plane->name;
>  			   __entry->frame = intel_crtc_get_vblank_counter(crtc);
>  			   __entry->scanline = intel_get_crtc_scanline(crtc);
>  			   memcpy(__entry->src, &plane->state->src, sizeof(__entry->src));
> @@ -310,7 +310,7 @@ TRACE_EVENT(intel_update_plane,
>  			   ),
>  
>  	    TP_printk("pipe %c, plane %s, frame=%u, scanline=%u, " DRM_RECT_FP_FMT " -> " DRM_RECT_FMT,
> -		      pipe_name(__entry->pipe), __entry->name,
> +		      pipe_name(__entry->pipe), __get_str(name),
>  		      __entry->frame, __entry->scanline,
>  		      DRM_RECT_FP_ARG((const struct drm_rect *)__entry->src),
>  		      DRM_RECT_ARG((const struct drm_rect *)__entry->dst))
> @@ -322,20 +322,20 @@ TRACE_EVENT(intel_disable_plane,
>  
>  	    TP_STRUCT__entry(
>  			     __field(enum pipe, pipe)
> -			     __field(const char *, name)
>  			     __field(u32, frame)
>  			     __field(u32, scanline)
> +			     __string(name, plane->name)
>  			     ),
>  
>  	    TP_fast_assign(
> +			   __assign_str(name, plane->name);
>  			   __entry->pipe = crtc->pipe;
> -			   __entry->name = plane->name;
>  			   __entry->frame = intel_crtc_get_vblank_counter(crtc);
>  			   __entry->scanline = intel_get_crtc_scanline(crtc);
>  			   ),
>  
>  	    TP_printk("pipe %c, plane %s, frame=%u, scanline=%u",
> -		      pipe_name(__entry->pipe), __entry->name,
> +		      pipe_name(__entry->pipe), __get_str(name),
>  		      __entry->frame, __entry->scanline)
>  );
>  
> -- 
> 2.21.0

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-07-11 19:55 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-10 16:01 [PATCH] drm/i915: Copy name string into ring buffer for intel_update/disable_plane tracepoints Steven Rostedt
2019-07-10 17:12 ` [PATCH v2] " Ville Syrjala
2019-07-11 19:55   ` Ville Syrjälä

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