Re: [mlankhorst:locking-rework 61/63] drivers/gpu/drm/i915/gem/i915_gem_userptr.c:315 i915_gem_object_userptr_submit_init() error: double locked 'i915->mm.notifier_lock' (orig line 270)

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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

Op 13-10-2020 om 14:27 schreef Dan Carpenter:
> tree:   git://people.freedesktop.org/~mlankhorst/linux locking-rework
> head:   a7bb9c136fffddfb2e1c5132deb1682c0d1f9fc9
> commit: d2902d100d21d7af1cea25975007e7ac4e03ab4c [61/63] drm/i915: Keep userpointer bindings if seqcount is unchanged
> config: i386-randconfig-m021-20201013 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> New smatch warnings:
> drivers/gpu/drm/i915/gem/i915_gem_userptr.c:315 i915_gem_object_userptr_submit_init() error: double locked 'i915->mm.notifier_lock' (orig line 270)
>
> vim +315 drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>
> f53270c47078c8 Maarten Lankhorst 2020-07-10  250  int i915_gem_object_userptr_submit_init(struct drm_i915_gem_object *obj)
> f53270c47078c8 Maarten Lankhorst 2020-07-10  251  {
> f53270c47078c8 Maarten Lankhorst 2020-07-10  252  	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> f53270c47078c8 Maarten Lankhorst 2020-07-10  253  	const unsigned long num_pages = obj->base.size >> PAGE_SHIFT;
> f53270c47078c8 Maarten Lankhorst 2020-07-10  254  	struct page **pvec;
> f53270c47078c8 Maarten Lankhorst 2020-07-10  255  	unsigned int gup_flags = 0;
> f53270c47078c8 Maarten Lankhorst 2020-07-10  256  	unsigned long notifier_seq;
> f53270c47078c8 Maarten Lankhorst 2020-07-10  257  	int pinned, ret;
> f53270c47078c8 Maarten Lankhorst 2020-07-10  258  
> f53270c47078c8 Maarten Lankhorst 2020-07-10  259  	if (obj->userptr.notifier.mm != current->mm)
> f53270c47078c8 Maarten Lankhorst 2020-07-10  260  		return -EFAULT;
> f53270c47078c8 Maarten Lankhorst 2020-07-10  261  
> f53270c47078c8 Maarten Lankhorst 2020-07-10  262  	ret = i915_gem_object_lock_interruptible(obj, NULL);
> f53270c47078c8 Maarten Lankhorst 2020-07-10  263  	if (ret)
> f53270c47078c8 Maarten Lankhorst 2020-07-10  264  		return ret;
> f53270c47078c8 Maarten Lankhorst 2020-07-10  265  
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  266  	/* optimistically try to preserve current pages while unlocked */
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  267  	if (i915_gem_object_has_pages(obj) &&
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  268  	    !mmu_interval_check_retry(&obj->userptr.notifier,
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  269  				      obj->userptr.notifier_seq)) {
> d2902d100d21d7 Maarten Lankhorst 2020-10-01 @270  		ret = mutex_lock_interruptible(&i915->mm.notifier_lock);
>                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This code looks intentional, but I don't 100% understand it so I'm just
> going to forward the zero day bot warning.
>
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  271  		if (!ret) {
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  272  			if (obj->userptr.pvec &&
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  273  			    !mmu_interval_read_retry(&obj->userptr.notifier,
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  274  						     obj->userptr.notifier_seq)) {
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  275  				obj->userptr.page_ref++;
> d2902d100d21d7 Maarten Lankhorst 2020-10-01  276  				mutex_unlock(&i915->mm.notifier_lock);
>
> Unlocked.

Yeah, the unlock needs to be moved up one branch, thanks for reporting. :)

We need to always unlock it, as it's used way inside core mm through mmu notifiers.

As the notifier_lock only needs to be held for extremely short amounts of time, and we cannot do any memory allocations. I think I'll replace it with a spinlock instead.

~Maarten