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.
Signed-off-by: Christian König <[email protected]>
---
drivers/gpu/drm/drm_syncobj.c | 7 +++++++
include/linux/lockdep.h | 5 +++++
2 files changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 6e74e6745eca..f51458615158 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -387,6 +387,13 @@ 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)
+ lockdep_assert_none_held_once();
+
*fence = drm_syncobj_fence_get(syncobj);
drm_syncobj_put(syncobj);
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)
--
2.25.1
Hi,
Just a comment about the comments:
On 1/18/21 10:03 AM, Christian König 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.
>
> Signed-off-by: Christian König <[email protected]>
> ---
> drivers/gpu/drm/drm_syncobj.c | 7 +++++++
> include/linux/lockdep.h | 5 +++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 6e74e6745eca..f51458615158 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -387,6 +387,13 @@ 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
held because
> + * trivial deadlock with page faults for example. Make lockdep complain
trivially
> + * about it early on.
> + */
> + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
> + lockdep_assert_none_held_once();
> +
> *fence = drm_syncobj_fence_get(syncobj);
> drm_syncobj_put(syncobj);
>
thanks.
--
~Randy
You can't do anything without having to do something else first.
-- Belefant's Law
On Mon, Jan 18, 2021 at 07:03:34PM +0100, Christian K?nig wrote:
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 6e74e6745eca..f51458615158 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -387,6 +387,13 @@ 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.
> + */
Egads, the cursed comment style is spreading :/
> + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
> + lockdep_assert_none_held_once();
> +
Should this not be part of drm_syncobj_fence_add_wait() instead? Also,
do you want to sprinkle might_sleep() around ?
> *fence = drm_syncobj_fence_get(syncobj);
> drm_syncobj_put(syncobj);
>
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
Am 19.01.21 um 10:35 schrieb Peter Zijlstra:
> On Mon, Jan 18, 2021 at 07:03:34PM +0100, Christian König wrote:
>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 6e74e6745eca..f51458615158 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -387,6 +387,13 @@ 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.
>> + */
> Egads, the cursed comment style is spreading :/
>
>> + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
>> + lockdep_assert_none_held_once();
>> +
> Should this not be part of drm_syncobj_fence_add_wait() instead?
drm_syncobj_fence_add_wait() is only called when the previous try of
finding the fence wasn't successfully.
If we want to check drivers for stupid behavior for the uncommon wait
before signal case we need this much earlier.
But I'm going to double check if drm_syncobj_fence_add_wait() isn't used
elsewhere as well.
> Also, do you want to sprinkle might_sleep() around ?
Good point. Going to add that as well.
Thanks,
Christian.
>
>> *fence = drm_syncobj_fence_get(syncobj);
>> drm_syncobj_put(syncobj);
>>
>> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
On Tue, Jan 19, 2021 at 10:46:53AM +0100, Christian K?nig wrote:
> But I'm going to double check if drm_syncobj_fence_add_wait() isn't used
> elsewhere as well.
drm_syncobj_array_wait_timeout()
Which is why I asked.. :-)
Am 19.01.21 um 11:05 schrieb Peter Zijlstra:
> On Tue, Jan 19, 2021 at 10:46:53AM +0100, Christian König wrote:
>> But I'm going to double check if drm_syncobj_fence_add_wait() isn't used
>> elsewhere as well.
> drm_syncobj_array_wait_timeout()
>
> Which is why I asked.. :-)
Ok, good point as well. But this isn't driver interface and rather IOCTL
implementation.
So if we hold any locks there it is our own stupidity. Going to add the
appropriate annotation anyway.
Thanks,
Christian.