[RFC] Spurious error handling code in 'vc4_get_hang_state_ioctl'

[RFC] Spurious error handling code in 'vc4_get_hang_state_ioctl'

[Christophe JAILLET]

Hi,

While looking for labels with a spurious order in the linux kernel code, 
I came across code around:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/vc4/vc4_gem.c#n106

The error handling in this for loop looks spurious to me:
    - if 'drm_gem_handle_create' fails, we leak 'bo_state'
      This is freed after the 'copy_to_user'
    - if 'drm_gem_handle_create' fails, then we have 'state->bo_count = 
i - 1'
      This looks odd. 'state->bo_count = i' sounds more logical to me.
    - as 'state->bo_count' is updated, this could mean than the 
'copy_to_user' should be performed.

I don't have any idea if one of the above is true (unless the memory 
leak), so I just send it as a RFC to get feed-back.

The corresponding patch would look like:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -109,18 +109,19 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, 
void *data,

          ret = drm_gem_handle_create(file_priv, kernel_state->bo[i],
                          &handle);

          if (ret) {
-            state->bo_count = i - 1;
-            goto err;
+            state->bo_count = i;
+            goto do_copy;
          }
          bo_state[i].handle = handle;
          bo_state[i].paddr = vc4_bo->base.paddr;
          bo_state[i].size = vc4_bo->base.base.size;
      }

+do_copy:
      if (copy_to_user((void __user *)(uintptr_t)get_state->bo,
               bo_state,
               state->bo_count * sizeof(*bo_state)))
          ret = -EFAULT;

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Best regards,

CJ

Re: [RFC] Spurious error handling code in 'vc4_get_hang_state_ioctl'

[Dan Carpenter]

On Mon, May 08, 2017 at 05:29:00PM +0200, Christophe JAILLET wrote:
> Hi,
> 
> While looking for labels with a spurious order in the linux kernel code, I
> came across code around:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/vc4/vc4_gem.c#n106
> 
> The error handling in this for loop looks spurious to me:
>    - if 'drm_gem_handle_create' fails, we leak 'bo_state'
>      This is freed after the 'copy_to_user'
>    - if 'drm_gem_handle_create' fails, then we have 'state->bo_count = i - 1'
>      This looks odd. 'state->bo_count = i' sounds more logical to me.

This is correct.

>    - as 'state->bo_count' is updated, this could mean than the
> 'copy_to_user' should be performed.

This also seems correct, but I doubt user space is actually set up to
handle errors in real life...  It could be that I'm too pesimistic.

regards,
dan carpenter

Re: [RFC] Spurious error handling code in 'vc4_get_hang_state_ioctl'

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:

> Hi,
>
> While looking for labels with a spurious order in the linux kernel code, 
> I came across code around:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/vc4/vc4_gem.c#n106
>
> The error handling in this for loop looks spurious to me:
>     - if 'drm_gem_handle_create' fails, we leak 'bo_state'
>       This is freed after the 'copy_to_user'
>     - if 'drm_gem_handle_create' fails, then we have 'state->bo_count = 
> i - 1'
>       This looks odd. 'state->bo_count = i' sounds more logical to me.
>     - as 'state->bo_count' is updated, this could mean than the 
> 'copy_to_user' should be performed.

The point of reducing the bo_count was presumably for freeing the
handles we had created before returning the error, which I forgot to
actually do (userspace won't know about the new handles to go and free
them itself).  This is not that big a deal, as this interface is for use
by a root-only, one-shot app that gets the hang state and dumps it to
disk, so it can be processed later to try to debug the hang.

Want to put together a patch that frees the handles (and bo_state),
though, for correctness?  Don't worry about copying any partial data out
on error -- the userspace tool just exits.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]