lkml.org 
[lkml]   [2009]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [Bug #12491] i915 lockdep warning
    Date
    [intel-gfx CC added]

    > I tested this on my 965 based system that showed the issue and it seemed resolved, at least no warnings in dmesg. Lockdep was confirmed to still be enabled.

    > I can mark the bug fixed in korg bugzilla too.

    Probably we just want to add a comment saying the patch works -- the bug
    should stay open until we get the fix upstream I guess.

    > Thank you Roland for looking into this, good work!

    Thanks for testing! Patch with updated changelog (including your
    tested-by) below. Jesse [Barnes] and/or Eric please review and apply:

    ---

    i915: Fix potential AB-BA deadlock in i915_gem_execbuffer()

    Lockdep warns that i915_gem_execbuffer() can trigger a page fault (which
    takes mmap_sem) while holding dev->struct_mutex, while drm_vm_open()
    (which is called with mmap_sem already held) takes dev->struct_mutex.
    So this is a potential AB-BA deadlock.

    The way that i915_gem_execbuffer() triggers a page fault is by doing
    copy_to_user() when returning new buffer offsets back to userspace;
    however there is no reason to hold the struct_mutex when doing this
    copy, since what is being copied is the contents of an array private to
    i915_gem_execbuffer() anyway. So we can fix the potential deadlock (and
    get rid of the lockdep warning) by simply moving the copy_to_user()
    outside of where struct_mutex is held.

    This fixes <http://bugzilla.kernel.org/show_bug.cgi?id=12491>.

    Reported-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
    Tested-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
    Signed-off-by: Roland Dreier <rolandd@cisco.com>
    ---
    drivers/gpu/drm/i915/i915_gem.c | 21 ++++++++++++---------
    1 files changed, 12 insertions(+), 9 deletions(-)

    diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
    index debad5c..23aad8c 100644
    --- a/drivers/gpu/drm/i915/i915_gem.c
    +++ b/drivers/gpu/drm/i915/i915_gem.c
    @@ -2610,15 +2610,6 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,

    i915_verify_inactive(dev, __FILE__, __LINE__);

    - /* Copy the new buffer offsets back to the user's exec list. */
    - ret = copy_to_user((struct drm_i915_relocation_entry __user *)
    - (uintptr_t) args->buffers_ptr,
    - exec_list,
    - sizeof(*exec_list) * args->buffer_count);
    - if (ret)
    - DRM_ERROR("failed to copy %d exec entries "
    - "back to user (%d)\n",
    - args->buffer_count, ret);
    err:
    for (i = 0; i < pinned; i++)
    i915_gem_object_unpin(object_list[i]);
    @@ -2628,6 +2619,18 @@ err:

    mutex_unlock(&dev->struct_mutex);

    + if (!ret) {
    + /* Copy the new buffer offsets back to the user's exec list. */
    + ret = copy_to_user((struct drm_i915_relocation_entry __user *)
    + (uintptr_t) args->buffers_ptr,
    + exec_list,
    + sizeof(*exec_list) * args->buffer_count);
    + if (ret)
    + DRM_ERROR("failed to copy %d exec entries "
    + "back to user (%d)\n",
    + args->buffer_count, ret);
    + }
    +
    pre_mutex_err:
    drm_free(object_list, sizeof(*object_list) * args->buffer_count,
    DRM_MEM_DRIVER);

    \
     
     \ /
      Last update: 2009-02-07 02:51    [W:0.023 / U:140.056 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site