On Thu, Jul 22, 2021 at 12:38:10PM +0200, Daniel Vetter wrote:

On Thu, Jul 22, 2021 at 05:29:27PM +0800, Desmond Cheong Zhi Xi wrote:

quoted

Inside drm_is_current_master, using the outer drm_device.master_mutex
to protect reads of drm_file.master makes the function prone to creating
lock hierarchy inversions. Instead, we can use the
drm_file.master_lookup_lock that sits at the bottom of the lock
hierarchy.

Reported-by: Daniel Vetter <redacted>
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
 drivers/gpu/drm/drm_auth.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index f00354bec3fb..9c24b8cc8e36 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c

@@ -63,8 +63,9 @@
 
 static bool drm_is_current_master_locked(struct drm_file *fpriv)
 {
-	lockdep_assert_held_once(&fpriv->minor->dev->master_mutex);
-
+	/* Either drm_device.master_mutex or drm_file.master_lookup_lock
+	 * should be held here.
+	 */

Disappointing that lockdep can't check or conditions for us, a
lockdep_assert_held_either would be really neat in some cases.

The implementation is not hard but I don't understand the usage, for
example, if we have a global variable x, and two locks L1 and L2, and
the function

	void do_something_to_x(void)
	{
		lockdep_assert_held_either(L1, L2);
		x++;
	}

and two call sites:

	void f(void)
	{
		lock(L1);
		do_something_to_x();
		unlock(L1);
	}

	void g(void)
	{
		lock(L2);
		do_something_to_x();
		unlock(L2);
	}

, wouldn't it be racy if f() and g() called by two threads at the same
time? Usually I would expect there exists a third synchronazition
mechanism (say M), which synchronizes the calls to f() and g(), and we
put M in the lockdep_assert_held() check inside do_something_to_x()
like:

	void do_something_to_x(void)
	{
		lockdep_assert_held_once(M);
		x++;
	}

But of course, M may not be a lock, so we cannot put the assert there.

My cscope failed to find ->master_lookup_lock in -rc2 and seems it's not
introduced in the patchset either, could you point me the branch this
patchset is based on, so that I could understand this better, and maybe
come up with a solution? Thanks ;-)

Regards,
Boqun

Adding lockdep folks, maybe they have ideas.

On the patch:

Reviewed-by: Daniel Vetter <redacted>

quoted

 	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
 }
 

@@ -82,9 +83,9 @@ bool drm_is_current_master(struct drm_file *fpriv)
 {
 	bool ret;
 
-	mutex_lock(&fpriv->minor->dev->master_mutex);
+	spin_lock(&fpriv->master_lookup_lock);
 	ret = drm_is_current_master_locked(fpriv);
-	mutex_unlock(&fpriv->minor->dev->master_mutex);
+	spin_unlock(&fpriv->master_lookup_lock);
 
 	return ret;
 }

-- 
2.25.1

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees