[Adding CCs.]
On Wednesday 04 February 2009, Roland Dreier wrote: > > Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=12491 > > Subject : i915 lockdep warning > > Submitter : Brandeburg, Jesse <[email protected]> > > Date : 2009-01-13 23:17 (23 days old) > > References : http://marc.info/?l=linux-kernel&m=123188898423532&w=4 > > Looking at the code, it seems that the issue is that the DRM > struct_mutex must be taken inside mmap_sem (because struct_mutex is > taken in drm_vm_open(), which is called with mmap_sem already held), but > i915_gem_execbuffer() does a copy_to_user() while holding struct_mutex, > and if this copy faults, then the VM tries to acquire mmap_sem -- ie > lockdep identifies correctly a potential AB/BA deadlock. > > I don't pretend to fully understand the DRM or GEM, but a possible fix > is below -- would be worth it to test and review, and get into 2.6.29 if > it is a correct fix: > > --- > 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 a private array 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 <[email protected]> > Signed-off-by: Roland Dreier <[email protected]> > --- > 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); > -- -- To unsubscribe from this list: send the line "unsubscribe kernel-testers" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
