| Message ID | 20210119114841.2003-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/syncobj: make lockdep complain on WAIT_FOR_SUBMIT v3 | expand |
On Tue, Jan 19, 2021 at 12:48 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT can't be used when we hold locks > since we are basically waiting for userspace to do something. > > Holding a lock while doing so can trivial deadlock with page faults > etc... > > So make lockdep complain when a driver tries to do this. > > v2: Add lockdep_assert_none_held() macro. > v3: Add might_sleep() and also use lockdep_assert_none_held() in the > IOCTL path. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/drm_syncobj.c | 12 ++++++++++++ > include/linux/lockdep.h | 5 +++++ > 2 files changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 6e74e6745eca..9ecfec1e5567 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -387,6 +387,15 @@ int drm_syncobj_find_fence(struct drm_file *file_private, > if (!syncobj) > return -ENOENT; > > + /* Waiting for userspace with locks help is illegal cause that can > + * trivial deadlock with page faults for example. Make lockdep complain > + * about it early on. > + */ > + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { > + might_sleep(); > + lockdep_assert_none_held_once(); > + } > + > *fence = drm_syncobj_fence_get(syncobj); > drm_syncobj_put(syncobj); I noticed a use-after-free here that goes back to 5.2, sending out a patch for that. > > @@ -940,6 +949,9 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs, > uint64_t *points; > uint32_t signaled_count, i; > > + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) > + lockdep_assert_none_held_once(); > + > points = kmalloc_array(count, sizeof(*points), GFP_KERNEL); > if (points == NULL) > return -ENOMEM; > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h > index b9e9adec73e8..6eb117c0d0f3 100644 > --- a/include/linux/lockdep.h > +++ b/include/linux/lockdep.h > @@ -310,6 +310,10 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); > WARN_ON_ONCE(debug_locks && !lockdep_is_held(l)); \ > } while (0) > > +#define lockdep_assert_none_held_once() do { \ > + WARN_ON_ONCE(debug_locks && current->lockdep_depth); \ > + } while (0) > + > #define lockdep_recursing(tsk) ((tsk)->lockdep_recursion) > > #define lockdep_pin_lock(l) lock_pin_lock(&(l)->dep_map) > @@ -387,6 +391,7 @@ extern int lockdep_is_held(const void *); > #define lockdep_assert_held_write(l) do { (void)(l); } while (0) > #define lockdep_assert_held_read(l) do { (void)(l); } while (0) > #define lockdep_assert_held_once(l) do { (void)(l); } while (0) > +#define lockdep_assert_none_held_once() do { } while (0) > > #define lockdep_recursing(tsk) (0) ofc needs ack from Peter, but drm parts look all good to me. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Tue, Jan 19, 2021 at 02:05:09PM +0100, Daniel Vetter wrote: > > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h > > index b9e9adec73e8..6eb117c0d0f3 100644 > > --- a/include/linux/lockdep.h > > +++ b/include/linux/lockdep.h > > @@ -310,6 +310,10 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); > > WARN_ON_ONCE(debug_locks && !lockdep_is_held(l)); \ > > } while (0) > > > > +#define lockdep_assert_none_held_once() do { \ > > + WARN_ON_ONCE(debug_locks && current->lockdep_depth); \ > > + } while (0) > > + > > #define lockdep_recursing(tsk) ((tsk)->lockdep_recursion) > > > > #define lockdep_pin_lock(l) lock_pin_lock(&(l)->dep_map) > > @@ -387,6 +391,7 @@ extern int lockdep_is_held(const void *); > > #define lockdep_assert_held_write(l) do { (void)(l); } while (0) > > #define lockdep_assert_held_read(l) do { (void)(l); } while (0) > > #define lockdep_assert_held_once(l) do { (void)(l); } while (0) > > +#define lockdep_assert_none_held_once() do { } while (0) > > > > #define lockdep_recursing(tsk) (0) > > ofc needs ack from Peter, but drm parts look all good to me. Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 6e74e6745eca..9ecfec1e5567 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -387,6 +387,15 @@ int drm_syncobj_find_fence(struct drm_file *file_private, if (!syncobj) return -ENOENT; + /* Waiting for userspace with locks help is illegal cause that can + * trivial deadlock with page faults for example. Make lockdep complain + * about it early on. + */ + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { + might_sleep(); + lockdep_assert_none_held_once(); + } + *fence = drm_syncobj_fence_get(syncobj); drm_syncobj_put(syncobj); @@ -940,6 +949,9 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs, uint64_t *points; uint32_t signaled_count, i; + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) + lockdep_assert_none_held_once(); + points = kmalloc_array(count, sizeof(*points), GFP_KERNEL); if (points == NULL) return -ENOMEM; diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index b9e9adec73e8..6eb117c0d0f3 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -310,6 +310,10 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); WARN_ON_ONCE(debug_locks && !lockdep_is_held(l)); \ } while (0) +#define lockdep_assert_none_held_once() do { \ + WARN_ON_ONCE(debug_locks && current->lockdep_depth); \ + } while (0) + #define lockdep_recursing(tsk) ((tsk)->lockdep_recursion) #define lockdep_pin_lock(l) lock_pin_lock(&(l)->dep_map) @@ -387,6 +391,7 @@ extern int lockdep_is_held(const void *); #define lockdep_assert_held_write(l) do { (void)(l); } while (0) #define lockdep_assert_held_read(l) do { (void)(l); } while (0) #define lockdep_assert_held_once(l) do { (void)(l); } while (0) +#define lockdep_assert_none_held_once() do { } while (0) #define lockdep_recursing(tsk) (0)
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT can't be used when we hold locks since we are basically waiting for userspace to do something. Holding a lock while doing so can trivial deadlock with page faults etc... So make lockdep complain when a driver tries to do this. v2: Add lockdep_assert_none_held() macro. v3: Add might_sleep() and also use lockdep_assert_none_held() in the IOCTL path. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_syncobj.c | 12 ++++++++++++ include/linux/lockdep.h | 5 +++++ 2 files changed, 17 insertions(+)