Re: [PATCH] drm/i915: Break up the large vsnprintf() in print_error_buffers()

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Jun 29, 2013 at 11:26:50PM +0100, Chris Wilson wrote:
> So it appears that I have encountered some bogosity when trying to call
> i915_error_printf() with many arguments from print_error_buffers(). The
> symptom is that the vsnprintf parser tries to interpret an integer arg
> as a character string, the resulting OOPS indicating stack corruption.
> Replacing the single call with its 13 format specifiers and arguments
> with multiple calls to i915_error_printf() worked fine. This patch goes
> one step further and introduced i915_error_puts() to pass the strings
> simply.
> 
> It may not fix the root cause, but it does prevent my box from dying and
> I think helps make print_error_buffers() more friendly.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=66077
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>

Queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 109 ++++++++++++++++++++++++++----------
>  1 file changed, 79 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 55c89af..7eae6b0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -650,41 +650,44 @@ static const char *purgeable_flag(int purgeable)
>  	return purgeable ? " purgeable" : "";
>  }
>  
> -static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
> -			       const char *f, va_list args)
> +static bool __i915_error_ok(struct drm_i915_error_state_buf *e)
>  {
> -	unsigned len;
>  
>  	if (!e->err && WARN(e->bytes > (e->size - 1), "overflow")) {
>  		e->err = -ENOSPC;
> -		return;
> +		return false;
>  	}
>  
>  	if (e->bytes == e->size - 1 || e->err)
> -		return;
> +		return false;
>  
> -	/* Seek the first printf which is hits start position */
> -	if (e->pos < e->start) {
> -		len = vsnprintf(NULL, 0, f, args);
> -		if (e->pos + len <= e->start) {
> -			e->pos += len;
> -			return;
> -		}
> +	return true;
> +}
>  
> -		/* First vsnprintf needs to fit in full for memmove*/
> -		if (len >= e->size) {
> -			e->err = -EIO;
> -			return;
> -		}
> +static bool __i915_error_seek(struct drm_i915_error_state_buf *e,
> +			      unsigned len)
> +{
> +	if (e->pos + len <= e->start) {
> +		e->pos += len;
> +		return false;
>  	}
>  
> -	len = vsnprintf(e->buf + e->bytes, e->size - e->bytes, f, args);
> -	if (len >= e->size - e->bytes)
> -		len = e->size - e->bytes - 1;
> +	/* First vsnprintf needs to fit in its entirety for memmove */
> +	if (len >= e->size) {
> +		e->err = -EIO;
> +		return false;
> +	}
>  
> +	return true;
> +}
> +
> +static void __i915_error_advance(struct drm_i915_error_state_buf *e,
> +				 unsigned len)
> +{
>  	/* If this is first printf in this window, adjust it so that
>  	 * start position matches start of the buffer
>  	 */
> +
>  	if (e->pos < e->start) {
>  		const size_t off = e->start - e->pos;
>  
> @@ -704,6 +707,51 @@ static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
>  	e->pos += len;
>  }
>  
> +static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
> +			       const char *f, va_list args)
> +{
> +	unsigned len;
> +
> +	if (!__i915_error_ok(e))
> +		return;
> +
> +	/* Seek the first printf which is hits start position */
> +	if (e->pos < e->start) {
> +		len = vsnprintf(NULL, 0, f, args);
> +		if (!__i915_error_seek(e, len))
> +			return;
> +	}
> +
> +	len = vsnprintf(e->buf + e->bytes, e->size - e->bytes, f, args);
> +	if (len >= e->size - e->bytes)
> +		len = e->size - e->bytes - 1;
> +
> +	__i915_error_advance(e, len);
> +}
> +
> +static void i915_error_puts(struct drm_i915_error_state_buf *e,
> +			    const char *str)
> +{
> +	unsigned len;
> +
> +	if (!__i915_error_ok(e))
> +		return;
> +
> +	len = strlen(str);
> +
> +	/* Seek the first printf which is hits start position */
> +	if (e->pos < e->start) {
> +		if (!__i915_error_seek(e, len))
> +			return;
> +	}
> +
> +	if (len >= e->size - e->bytes)
> +		len = e->size - e->bytes - 1;
> +	memcpy(e->buf + e->bytes, str, len);
> +
> +	__i915_error_advance(e, len);
> +}
> +
>  void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...)
>  {
>  	va_list args;
> @@ -714,6 +762,7 @@ void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...)
>  }
>  
>  #define err_printf(e, ...) i915_error_printf(e, __VA_ARGS__)
> +#define err_puts(e, s) i915_error_puts(e, s)
>  
>  static void print_error_buffers(struct drm_i915_error_state_buf *m,
>  				const char *name,
> @@ -723,26 +772,26 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m,
>  	err_printf(m, "%s [%d]:\n", name, count);
>  
>  	while (count--) {
> -		err_printf(m, "  %08x %8u %02x %02x %x %x%s%s%s%s%s%s%s",
> +		err_printf(m, "  %08x %8u %02x %02x %x %x",
>  			   err->gtt_offset,
>  			   err->size,
>  			   err->read_domains,
>  			   err->write_domain,
> -			   err->rseqno, err->wseqno,
> -			   pin_flag(err->pinned),
> -			   tiling_flag(err->tiling),
> -			   dirty_flag(err->dirty),
> -			   purgeable_flag(err->purgeable),
> -			   err->ring != -1 ? " " : "",
> -			   ring_str(err->ring),
> -			   cache_level_str(err->cache_level));
> +			   err->rseqno, err->wseqno);
> +		err_puts(m, pin_flag(err->pinned));
> +		err_puts(m, tiling_flag(err->tiling));
> +		err_puts(m, dirty_flag(err->dirty));
> +		err_puts(m, purgeable_flag(err->purgeable));
> +		err_puts(m, err->ring != -1 ? " " : "");
> +		err_puts(m, ring_str(err->ring));
> +		err_puts(m, cache_level_str(err->cache_level));
>  
>  		if (err->name)
>  			err_printf(m, " (name: %d)", err->name);
>  		if (err->fence_reg != I915_FENCE_REG_NONE)
>  			err_printf(m, " (fence: %d)", err->fence_reg);
>  
> -		err_printf(m, "\n");
> +		err_puts(m, "\n");
>  		err++;
>  	}
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch