Let lockdep complain when locks are taken while waiting for userspace.

Let lockdep complain when locks are taken while waiting for userspace.

[Christian König]

Hi guys,

because of the Vulkan graphics API we have a specialized synchronization object to handle both inter process as well as process to hardware synchronization.

The problem is now that when drivers call this interface with some lock help it is trivial to create a deadlock when those locks are also needed in a page fault for example.

The idea of this patch is now to let lockdep complain when any locks are called when the interface is used.

Please review and/or comment.

Thanks,
Christian.

[PATCH] drm/syncobj: make lockdep complain on WAIT_FOR_SUBMIT v2

[Christian König]

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 <christian.koenig@amd.com>
---
 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

Re: [PATCH] drm/syncobj: make lockdep complain on WAIT_FOR_SUBMIT v2

[Randy Dunlap]

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 <christian.koenig@amd.com>
> ---
>  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

Re: [PATCH] drm/syncobj: make lockdep complain on WAIT_FOR_SUBMIT v2

[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? 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

Re: [PATCH] drm/syncobj: make lockdep complain on WAIT_FOR_SUBMIT v2

[Christian König]

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

Re: [PATCH] drm/syncobj: make lockdep complain on WAIT_FOR_SUBMIT v2

[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.. :-)

Re: [PATCH] drm/syncobj: make lockdep complain on WAIT_FOR_SUBMIT v2

[Christian König]

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.