[Intel-gfx] [PATCH] drm/i915: Trim the i915_error_printf by the trailing '\0'

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> It seems the request to vsnprintf will try to write to the byte past the
> end of the maximum buffer, so trim the length by one byte.
>
> [76973.700434] BUG: unable to handle kernel paging request at 0000000000001000
> [76973.700468] IP: [<ffffffff811fb84d>] strnlen+0xd/0x40
> [76973.700494] PGD 10387b067 PUD 15411a067 PMD 0
> [76973.700515] Oops: 0000 [#1] SMP
> [76973.700529] Modules linked in: cpufreq_userspace cpufreq_powersave cpufreq_stats cpufreq_conservative binfmt_misc nfs lockd fscache sunrpc coretemp crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 ablk_helper cryptd lrw gf128mul glue_helper iTCO_wdt iTCO_vendor_support microcode i2c_i801 lpc_ich mfd_core acpi_cpufreq mperf evdev processor ext4 crc16 jbd2 mbcache sd_mod crc_t10dif ahci libahci libata scsi_mod thermal fan
> [76973.700718] CPU: 2 PID: 13279 Comm: intel_error_dec Not tainted 3.10.0-rc6+ #99
> [76973.700746] Hardware name: Intel Corporation Shark Bay Client platform/Flathead Creek Crb, BIOS HSWLPTU1.86C.0109.R03.1301282055 01/28/2013
> [76973.700793] task: ffff880153fa2100 ti: ffff880152310000 task.ti: ffff880152310000
> [76973.700820] RIP: 0010:[<ffffffff811fb84d>]  [<ffffffff811fb84d>] strnlen+0xd/0x40
> [76973.700849] RSP: 0018:ffff880152311cb0  EFLAGS: 00010286
> [76973.700869] RAX: ffffffff81774000 RBX: ffff880154000072 RCX: fffffffffffffffe
> [76973.700895] RDX: 0000000000001000 RSI: ffffffffffffffff RDI: 0000000000001000
> [76973.700920] RBP: ffff880152311cb0 R08: 000000000000ffff R09: 000000000000ffff
> [76973.700948] R10: 0000000000000000 R11: 000000000000000f R12: 0000000000001000
> [76973.700974] R13: ffff880154001000 R14: 00000000ffffffff R15: 0000000000000000
> [76973.701000] FS:  00007f00fc942880(0000) GS:ffff88015e300000(0000) knlGS:0000000000000000
> [76973.701029] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [76973.701050] CR2: 0000000000001000 CR3: 000000015939a000 CR4: 00000000001407e0
> [76973.701075] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [76973.701103] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [76973.701128] Stack:
> [76973.701136]  ffff880152311ce8 ffffffff811fdafb ffff880154000072 ffff880154001000
> [76973.701167]  ffff880152311d58 ffffffff817a703e ffffffff817a703e ffff880152311d48
> [76973.701198]  ffffffff811fe8a9 0000000a0000ffff 0000000000000001 ffff880154000000
> [76973.701229] Call Trace:
> [76973.701242]  [<ffffffff811fdafb>] string.isra.5+0x3b/0xf0
> [76973.701264]  [<ffffffff811fe8a9>] vsnprintf+0x1d9/0x670
> [76973.701286]  [<ffffffff812d13c9>] i915_error_printf+0xb9/0x160
> [76973.701311]  [<ffffffff812d15fc>] print_error_buffers+0x18c/0x1e0
> [76973.701335]  [<ffffffff812d1a5e>] i915_error_state_read+0x40e/0x8e0
> [76973.701361]  [<ffffffff81114527>] vfs_read+0x97/0x160
> [76973.701381]  [<ffffffff81114f74>] SyS_read+0x44/0x90
> [76973.701403]  [<ffffffff8141a552>] system_call_fastpath+0x16/0x1b
> [76973.701424] Code: c0 01 80 38 00 75 f7 48 29 f8 5d c3 31 c0 5d c3 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 8d 4e ff 48 85 f6 48 89 e5 74 2a <80> 3f 00 74 25 48 89 f8 31 d2 eb 10 0f 1f 80 00 00 00 00 48 83
> [76973.701535] RIP  [<ffffffff811fb84d>] strnlen+0xd/0x40
> [76973.701550]  RSP <ffff880152311cb0>
> [76973.701559] CR2: 0000000000001000
>
> Regression from commit edc3d8848dc9fe2a470316363dab8ef211d77e01
> Author: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Date:   Thu May 23 13:55:35 2013 +0300
>
>     drm/i915: avoid big kmallocs on reading error state
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ec64f95..c436af9 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -653,7 +653,7 @@ static const char *purgeable_flag(int purgeable)
>  static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
>  			       const char *f, va_list args)
>  {
> -	unsigned len;
> +	int len, rem;
>  
>  	if (!e->err && WARN(e->bytes > (e->size - 1), "overflow")) {
>  		e->err = -ENOSPC;
> @@ -666,6 +666,11 @@ static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
>  	/* Seek the first printf which is hits start position */
>  	if (e->pos < e->start) {
>  		len = vsnprintf(NULL, 0, f, args);
> +		if (len == -1) {
> +			e->err = -EIO;
> +			return;
> +		}
> +

-1 from vsnprintf would be broken vsnprintf. <0 would be better
for paranoia.

>  		if (e->pos + len <= e->start) {
>  			e->pos += len;
>  			return;
> @@ -678,9 +683,14 @@ static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
>  		}
>  	}
>  
> -	len = vsnprintf(e->buf + e->bytes, e->size - e->bytes, f, args);
> -	if (len >= e->size - e->bytes)
> -		len = e->size - e->bytes - 1;

The returned len excludes the trailing null. Len is adjusted to omit
the forced trailing null we got if we were truncated. So i fail to see
how the existing code is broken in here.

> +	rem = e->size - e->bytes - 1;

e-size - e->bytes passed to vsnprintf includes the trailing null so no
need to adjust for it.

> +	len = vsnprintf(e->buf + e->bytes, rem, f, args);
> +	if (len == -1) {
> +		e->err = -EIO;
> +		return;
> +	}
> +	if (len > rem)
> +		len = rem;

-- Mika