Re: [PATCH] drm/i915: Avoid might_fault during pwrite whilst holdingour mutex

[Uwe Helm]

On Sun, Nov 7, 2010 at 5:34 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> ... and so prevent a potential circular reference:
>
>  [ INFO: possible circular locking dependency detected ]
>  2.6.37-rc1-uwe1+ #4
>  -------------------------------------------------------
>  Xorg/1401 is trying to acquire lock:
>   (&mm->mmap_sem){++++++}, at: [<c01e4ddb>] might_fault+0x4b/0xa0
>
>  but task is already holding lock:
>   (&dev->struct_mutex){+.+.+.}, at: [<f869c3ac>]
>  i915_mutex_lock_interruptible+0x3c/0x60 [i915]
>
>  which lock already depends on the new lock.
>
> When the locking around the pwrite ioctl was simplified, I did not spot
> that the phys path never took any locks and so we introduced this
> potential circular reference.
>
> Reported-by: Uwe Helm <uwe.helm@xxxxxxxxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   25 ++++++++++++++++---------
>  1 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 984eb6e..eba9b16 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4878,17 +4878,24 @@ i915_gem_phys_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
>                     struct drm_file *file_priv)
>  {
>        struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
> -       void *obj_addr;
> -       int ret;
> -       char __user *user_data;
> +       void *vaddr = obj_priv->phys_obj->handle->vaddr + args->offset;
> +       char __user *user_data = (char __user *) (uintptr_t) args->data_ptr;
>
> -       user_data = (char __user *) (uintptr_t) args->data_ptr;
> -       obj_addr = obj_priv->phys_obj->handle->vaddr + args->offset;
> +       DRM_DEBUG_DRIVER("vaddr %p, %lld\n", vaddr, args->size);
>
> -       DRM_DEBUG_DRIVER("obj_addr %p, %lld\n", obj_addr, args->size);
> -       ret = copy_from_user(obj_addr, user_data, args->size);
> -       if (ret)
> -               return -EFAULT;
> +       if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
> +               unsigned long unwritten;
> +
> +               /* The physical object once assigned is fixed for the lifetime
> +                * of the obj, so we can safely drop the lock and continue
> +                * to access vaddr.
> +                */
> +               mutex_unlock(&dev->struct_mutex);
> +               unwritten = copy_from_user(vaddr, user_data, args->size);
> +               mutex_lock(&dev->struct_mutex);
> +               if (unwritten)
> +                       return -EFAULT;
> +       }
>
>        drm_agp_chipset_flush(dev);
>        return 0;
> --
> 1.7.2.3
>
>

works, thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/