From: Dan Carpenter <dan.carpenter@oracle.com>
To: kbuild@lists.01.org
Subject: [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)
Date: Tue, 13 Oct 2020 15:27:41 +0300 [thread overview]
Message-ID: <20201013122741.GT1042@kadam> (raw)
[-- 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 --]
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: kbuild-all@lists.01.org
Subject: [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)
Date: Tue, 13 Oct 2020 15:27:41 +0300 [thread overview]
Message-ID: <20201013122741.GT1042@kadam> (raw)
[-- 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 --]
next reply other threads:[~2020-10-13 12:27 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-13 12:27 Dan Carpenter [this message]
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 14:25 ` Maarten Lankhorst
-- strict thread matches above, loose matches on Subject: below --
2020-10-13 11:25 kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201013122741.GT1042@kadam \
--to=dan.carpenter@oracle.com \
--cc=kbuild@lists.01.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.