* [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)
@ 2020-10-13 12:27 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-10-13 12:27 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 8499 bytes --]
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.
d2902d100d21d7 Maarten Lankhorst 2020-10-01 277
d2902d100d21d7 Maarten Lankhorst 2020-10-01 278 /* We can keep using the current binding, this is the fastpath */
d2902d100d21d7 Maarten Lankhorst 2020-10-01 279 ret = 1;
d2902d100d21d7 Maarten Lankhorst 2020-10-01 280 }
d2902d100d21d7 Maarten Lankhorst 2020-10-01 281 }
d2902d100d21d7 Maarten Lankhorst 2020-10-01 282 }
On the else patch, "ret == 0" and we didn't take the lock. On the
if condtion true path if ret is zero then we are still holding the lock.
d2902d100d21d7 Maarten Lankhorst 2020-10-01 283
d2902d100d21d7 Maarten Lankhorst 2020-10-01 284 if (!ret) {
^^^^
Check here.
f53270c47078c8 Maarten Lankhorst 2020-07-10 285 /* Make sure userptr is unbound for next attempt, so we don't use stale pages. */
f53270c47078c8 Maarten Lankhorst 2020-07-10 286 ret = i915_gem_object_userptr_unbind(obj, false);
d2902d100d21d7 Maarten Lankhorst 2020-10-01 287 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 288 i915_gem_object_unlock(obj);
d2902d100d21d7 Maarten Lankhorst 2020-10-01 289 if (ret < 0)
f53270c47078c8 Maarten Lankhorst 2020-07-10 290 return ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 291
d2902d100d21d7 Maarten Lankhorst 2020-10-01 292 if (ret > 0)
d2902d100d21d7 Maarten Lankhorst 2020-10-01 293 return 0;
d2902d100d21d7 Maarten Lankhorst 2020-10-01 294
f53270c47078c8 Maarten Lankhorst 2020-07-10 295 notifier_seq = mmu_interval_read_begin(&obj->userptr.notifier);
f53270c47078c8 Maarten Lankhorst 2020-07-10 296
f53270c47078c8 Maarten Lankhorst 2020-07-10 297 pvec = kvmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL);
f53270c47078c8 Maarten Lankhorst 2020-07-10 298 if (!pvec)
f53270c47078c8 Maarten Lankhorst 2020-07-10 299 return -ENOMEM;
f53270c47078c8 Maarten Lankhorst 2020-07-10 300
f53270c47078c8 Maarten Lankhorst 2020-07-10 301 if (!i915_gem_object_is_readonly(obj))
f53270c47078c8 Maarten Lankhorst 2020-07-10 302 gup_flags |= FOLL_WRITE;
f53270c47078c8 Maarten Lankhorst 2020-07-10 303
f53270c47078c8 Maarten Lankhorst 2020-07-10 304 pinned = ret = 0;
f53270c47078c8 Maarten Lankhorst 2020-07-10 305 while (pinned < num_pages) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 306 ret = pin_user_pages_fast(obj->userptr.ptr + pinned * PAGE_SIZE,
f53270c47078c8 Maarten Lankhorst 2020-07-10 307 num_pages - pinned, gup_flags,
f53270c47078c8 Maarten Lankhorst 2020-07-10 308 &pvec[pinned]);
f53270c47078c8 Maarten Lankhorst 2020-07-10 309 if (ret < 0)
f53270c47078c8 Maarten Lankhorst 2020-07-10 310 goto out;
f53270c47078c8 Maarten Lankhorst 2020-07-10 311
f53270c47078c8 Maarten Lankhorst 2020-07-10 312 pinned += ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 313 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 314
f53270c47078c8 Maarten Lankhorst 2020-07-10 @315 ret = mutex_lock_interruptible(&i915->mm.notifier_lock);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Potentially a double lock?
f53270c47078c8 Maarten Lankhorst 2020-07-10 316 if (ret)
f53270c47078c8 Maarten Lankhorst 2020-07-10 317 goto out;
f53270c47078c8 Maarten Lankhorst 2020-07-10 318
f53270c47078c8 Maarten Lankhorst 2020-07-10 319 if (mmu_interval_read_retry(&obj->userptr.notifier,
f53270c47078c8 Maarten Lankhorst 2020-07-10 320 !obj->userptr.page_ref ? notifier_seq :
f53270c47078c8 Maarten Lankhorst 2020-07-10 321 obj->userptr.notifier_seq)) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 322 ret = -EAGAIN;
f53270c47078c8 Maarten Lankhorst 2020-07-10 323 goto out_unlock;
f53270c47078c8 Maarten Lankhorst 2020-07-10 324 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 325
f53270c47078c8 Maarten Lankhorst 2020-07-10 326 if (!obj->userptr.page_ref++) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 327 obj->userptr.pvec = pvec;
f53270c47078c8 Maarten Lankhorst 2020-07-10 328 obj->userptr.notifier_seq = notifier_seq;
f53270c47078c8 Maarten Lankhorst 2020-07-10 329
f53270c47078c8 Maarten Lankhorst 2020-07-10 330 pvec = NULL;
f53270c47078c8 Maarten Lankhorst 2020-07-10 331 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 332
f53270c47078c8 Maarten Lankhorst 2020-07-10 333 out_unlock:
f53270c47078c8 Maarten Lankhorst 2020-07-10 334 mutex_unlock(&i915->mm.notifier_lock);
f53270c47078c8 Maarten Lankhorst 2020-07-10 335
f53270c47078c8 Maarten Lankhorst 2020-07-10 336 out:
f53270c47078c8 Maarten Lankhorst 2020-07-10 337 if (pvec) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 338 unpin_user_pages(pvec, pinned);
f53270c47078c8 Maarten Lankhorst 2020-07-10 339 kvfree(pvec);
f53270c47078c8 Maarten Lankhorst 2020-07-10 340 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 341
f53270c47078c8 Maarten Lankhorst 2020-07-10 342 return ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 343 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40948 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* [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)
@ 2020-10-13 12:27 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-10-13 12:27 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 8499 bytes --]
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.
d2902d100d21d7 Maarten Lankhorst 2020-10-01 277
d2902d100d21d7 Maarten Lankhorst 2020-10-01 278 /* We can keep using the current binding, this is the fastpath */
d2902d100d21d7 Maarten Lankhorst 2020-10-01 279 ret = 1;
d2902d100d21d7 Maarten Lankhorst 2020-10-01 280 }
d2902d100d21d7 Maarten Lankhorst 2020-10-01 281 }
d2902d100d21d7 Maarten Lankhorst 2020-10-01 282 }
On the else patch, "ret == 0" and we didn't take the lock. On the
if condtion true path if ret is zero then we are still holding the lock.
d2902d100d21d7 Maarten Lankhorst 2020-10-01 283
d2902d100d21d7 Maarten Lankhorst 2020-10-01 284 if (!ret) {
^^^^
Check here.
f53270c47078c8 Maarten Lankhorst 2020-07-10 285 /* Make sure userptr is unbound for next attempt, so we don't use stale pages. */
f53270c47078c8 Maarten Lankhorst 2020-07-10 286 ret = i915_gem_object_userptr_unbind(obj, false);
d2902d100d21d7 Maarten Lankhorst 2020-10-01 287 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 288 i915_gem_object_unlock(obj);
d2902d100d21d7 Maarten Lankhorst 2020-10-01 289 if (ret < 0)
f53270c47078c8 Maarten Lankhorst 2020-07-10 290 return ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 291
d2902d100d21d7 Maarten Lankhorst 2020-10-01 292 if (ret > 0)
d2902d100d21d7 Maarten Lankhorst 2020-10-01 293 return 0;
d2902d100d21d7 Maarten Lankhorst 2020-10-01 294
f53270c47078c8 Maarten Lankhorst 2020-07-10 295 notifier_seq = mmu_interval_read_begin(&obj->userptr.notifier);
f53270c47078c8 Maarten Lankhorst 2020-07-10 296
f53270c47078c8 Maarten Lankhorst 2020-07-10 297 pvec = kvmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL);
f53270c47078c8 Maarten Lankhorst 2020-07-10 298 if (!pvec)
f53270c47078c8 Maarten Lankhorst 2020-07-10 299 return -ENOMEM;
f53270c47078c8 Maarten Lankhorst 2020-07-10 300
f53270c47078c8 Maarten Lankhorst 2020-07-10 301 if (!i915_gem_object_is_readonly(obj))
f53270c47078c8 Maarten Lankhorst 2020-07-10 302 gup_flags |= FOLL_WRITE;
f53270c47078c8 Maarten Lankhorst 2020-07-10 303
f53270c47078c8 Maarten Lankhorst 2020-07-10 304 pinned = ret = 0;
f53270c47078c8 Maarten Lankhorst 2020-07-10 305 while (pinned < num_pages) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 306 ret = pin_user_pages_fast(obj->userptr.ptr + pinned * PAGE_SIZE,
f53270c47078c8 Maarten Lankhorst 2020-07-10 307 num_pages - pinned, gup_flags,
f53270c47078c8 Maarten Lankhorst 2020-07-10 308 &pvec[pinned]);
f53270c47078c8 Maarten Lankhorst 2020-07-10 309 if (ret < 0)
f53270c47078c8 Maarten Lankhorst 2020-07-10 310 goto out;
f53270c47078c8 Maarten Lankhorst 2020-07-10 311
f53270c47078c8 Maarten Lankhorst 2020-07-10 312 pinned += ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 313 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 314
f53270c47078c8 Maarten Lankhorst 2020-07-10 @315 ret = mutex_lock_interruptible(&i915->mm.notifier_lock);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Potentially a double lock?
f53270c47078c8 Maarten Lankhorst 2020-07-10 316 if (ret)
f53270c47078c8 Maarten Lankhorst 2020-07-10 317 goto out;
f53270c47078c8 Maarten Lankhorst 2020-07-10 318
f53270c47078c8 Maarten Lankhorst 2020-07-10 319 if (mmu_interval_read_retry(&obj->userptr.notifier,
f53270c47078c8 Maarten Lankhorst 2020-07-10 320 !obj->userptr.page_ref ? notifier_seq :
f53270c47078c8 Maarten Lankhorst 2020-07-10 321 obj->userptr.notifier_seq)) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 322 ret = -EAGAIN;
f53270c47078c8 Maarten Lankhorst 2020-07-10 323 goto out_unlock;
f53270c47078c8 Maarten Lankhorst 2020-07-10 324 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 325
f53270c47078c8 Maarten Lankhorst 2020-07-10 326 if (!obj->userptr.page_ref++) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 327 obj->userptr.pvec = pvec;
f53270c47078c8 Maarten Lankhorst 2020-07-10 328 obj->userptr.notifier_seq = notifier_seq;
f53270c47078c8 Maarten Lankhorst 2020-07-10 329
f53270c47078c8 Maarten Lankhorst 2020-07-10 330 pvec = NULL;
f53270c47078c8 Maarten Lankhorst 2020-07-10 331 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 332
f53270c47078c8 Maarten Lankhorst 2020-07-10 333 out_unlock:
f53270c47078c8 Maarten Lankhorst 2020-07-10 334 mutex_unlock(&i915->mm.notifier_lock);
f53270c47078c8 Maarten Lankhorst 2020-07-10 335
f53270c47078c8 Maarten Lankhorst 2020-07-10 336 out:
f53270c47078c8 Maarten Lankhorst 2020-07-10 337 if (pvec) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 338 unpin_user_pages(pvec, pinned);
f53270c47078c8 Maarten Lankhorst 2020-07-10 339 kvfree(pvec);
f53270c47078c8 Maarten Lankhorst 2020-07-10 340 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 341
f53270c47078c8 Maarten Lankhorst 2020-07-10 342 return ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 343 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40948 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* 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)
2020-10-13 12:27 ` Dan Carpenter
(?)
@ 2020-10-13 14:25 ` Maarten Lankhorst
-1 siblings, 0 replies; 4+ messages in thread
From: Maarten Lankhorst @ 2020-10-13 14:25 UTC (permalink / raw)
To: kbuild-all
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [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)
@ 2020-10-13 11:25 kernel test robot
0 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2020-10-13 11:25 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 8615 bytes --]
CC: kbuild-all(a)lists.01.org
TO: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
tree: git://people.freedesktop.org/~mlankhorst/linux locking-rework
head: a7bb9c136fffddfb2e1c5132deb1682c0d1f9fc9
commit: d2902d100d21d7af1cea25975007e7ac4e03ab4c [61/63] drm/i915: Keep userpointer bindings if seqcount is unchanged
:::::: branch date: 21 hours ago
:::::: commit date: 21 hours ago
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)
Old smatch warnings:
drivers/gpu/drm/i915/gem/i915_gem_object.h:146 __i915_gem_object_lock() error: we previously assumed 'ww' could be null (see line 138)
vim +315 drivers/gpu/drm/i915/gem/i915_gem_userptr.c
f53270c47078c8 Maarten Lankhorst 2020-07-10 249
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);
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);
d2902d100d21d7 Maarten Lankhorst 2020-10-01 277
d2902d100d21d7 Maarten Lankhorst 2020-10-01 278 /* We can keep using the current binding, this is the fastpath */
d2902d100d21d7 Maarten Lankhorst 2020-10-01 279 ret = 1;
d2902d100d21d7 Maarten Lankhorst 2020-10-01 280 }
d2902d100d21d7 Maarten Lankhorst 2020-10-01 281 }
d2902d100d21d7 Maarten Lankhorst 2020-10-01 282 }
d2902d100d21d7 Maarten Lankhorst 2020-10-01 283
d2902d100d21d7 Maarten Lankhorst 2020-10-01 284 if (!ret) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 285 /* Make sure userptr is unbound for next attempt, so we don't use stale pages. */
f53270c47078c8 Maarten Lankhorst 2020-07-10 286 ret = i915_gem_object_userptr_unbind(obj, false);
d2902d100d21d7 Maarten Lankhorst 2020-10-01 287 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 288 i915_gem_object_unlock(obj);
d2902d100d21d7 Maarten Lankhorst 2020-10-01 289 if (ret < 0)
f53270c47078c8 Maarten Lankhorst 2020-07-10 290 return ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 291
d2902d100d21d7 Maarten Lankhorst 2020-10-01 292 if (ret > 0)
d2902d100d21d7 Maarten Lankhorst 2020-10-01 293 return 0;
d2902d100d21d7 Maarten Lankhorst 2020-10-01 294
f53270c47078c8 Maarten Lankhorst 2020-07-10 295 notifier_seq = mmu_interval_read_begin(&obj->userptr.notifier);
f53270c47078c8 Maarten Lankhorst 2020-07-10 296
f53270c47078c8 Maarten Lankhorst 2020-07-10 297 pvec = kvmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL);
f53270c47078c8 Maarten Lankhorst 2020-07-10 298 if (!pvec)
f53270c47078c8 Maarten Lankhorst 2020-07-10 299 return -ENOMEM;
f53270c47078c8 Maarten Lankhorst 2020-07-10 300
f53270c47078c8 Maarten Lankhorst 2020-07-10 301 if (!i915_gem_object_is_readonly(obj))
f53270c47078c8 Maarten Lankhorst 2020-07-10 302 gup_flags |= FOLL_WRITE;
f53270c47078c8 Maarten Lankhorst 2020-07-10 303
f53270c47078c8 Maarten Lankhorst 2020-07-10 304 pinned = ret = 0;
f53270c47078c8 Maarten Lankhorst 2020-07-10 305 while (pinned < num_pages) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 306 ret = pin_user_pages_fast(obj->userptr.ptr + pinned * PAGE_SIZE,
f53270c47078c8 Maarten Lankhorst 2020-07-10 307 num_pages - pinned, gup_flags,
f53270c47078c8 Maarten Lankhorst 2020-07-10 308 &pvec[pinned]);
f53270c47078c8 Maarten Lankhorst 2020-07-10 309 if (ret < 0)
f53270c47078c8 Maarten Lankhorst 2020-07-10 310 goto out;
f53270c47078c8 Maarten Lankhorst 2020-07-10 311
f53270c47078c8 Maarten Lankhorst 2020-07-10 312 pinned += ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 313 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 314
f53270c47078c8 Maarten Lankhorst 2020-07-10 @315 ret = mutex_lock_interruptible(&i915->mm.notifier_lock);
f53270c47078c8 Maarten Lankhorst 2020-07-10 316 if (ret)
f53270c47078c8 Maarten Lankhorst 2020-07-10 317 goto out;
f53270c47078c8 Maarten Lankhorst 2020-07-10 318
f53270c47078c8 Maarten Lankhorst 2020-07-10 319 if (mmu_interval_read_retry(&obj->userptr.notifier,
f53270c47078c8 Maarten Lankhorst 2020-07-10 320 !obj->userptr.page_ref ? notifier_seq :
f53270c47078c8 Maarten Lankhorst 2020-07-10 321 obj->userptr.notifier_seq)) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 322 ret = -EAGAIN;
f53270c47078c8 Maarten Lankhorst 2020-07-10 323 goto out_unlock;
f53270c47078c8 Maarten Lankhorst 2020-07-10 324 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 325
f53270c47078c8 Maarten Lankhorst 2020-07-10 326 if (!obj->userptr.page_ref++) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 327 obj->userptr.pvec = pvec;
f53270c47078c8 Maarten Lankhorst 2020-07-10 328 obj->userptr.notifier_seq = notifier_seq;
f53270c47078c8 Maarten Lankhorst 2020-07-10 329
f53270c47078c8 Maarten Lankhorst 2020-07-10 330 pvec = NULL;
f53270c47078c8 Maarten Lankhorst 2020-07-10 331 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 332
f53270c47078c8 Maarten Lankhorst 2020-07-10 333 out_unlock:
f53270c47078c8 Maarten Lankhorst 2020-07-10 334 mutex_unlock(&i915->mm.notifier_lock);
f53270c47078c8 Maarten Lankhorst 2020-07-10 335
f53270c47078c8 Maarten Lankhorst 2020-07-10 336 out:
f53270c47078c8 Maarten Lankhorst 2020-07-10 337 if (pvec) {
f53270c47078c8 Maarten Lankhorst 2020-07-10 338 unpin_user_pages(pvec, pinned);
f53270c47078c8 Maarten Lankhorst 2020-07-10 339 kvfree(pvec);
f53270c47078c8 Maarten Lankhorst 2020-07-10 340 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 341
f53270c47078c8 Maarten Lankhorst 2020-07-10 342 return ret;
f53270c47078c8 Maarten Lankhorst 2020-07-10 343 }
f53270c47078c8 Maarten Lankhorst 2020-07-10 344
:::::: The code at line 315 was first introduced by commit
:::::: f53270c47078c87a476012e95049492ef7cf2038 drm/i915: Fix userptr so we do not have to worry about obj->mm.lock, v2.
:::::: TO: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
:::::: CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40948 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-13 14:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-13 12:27 [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) Dan Carpenter
2020-10-13 12:27 ` Dan Carpenter
2020-10-13 14:25 ` Maarten Lankhorst
-- strict thread matches above, loose matches on Subject: below --
2020-10-13 11:25 kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.