diff mbox series

[1/2] lockdep: add lockdep_assert_not_held()

Message ID 37a29c383bff2fb1605241ee6c7c9be3784fb3c6.1613171185.git.skhan@linuxfoundation.org
State New
Headers show
Series Add lockdep_assert_not_held() | expand

Commit Message

Shuah Khan Feb. 12, 2021, 11:28 p.m. UTC
Some kernel functions must not be called holding a specific lock. Doing
so could lead to locking problems. Currently these routines call
lock_is_held() to check for lock hold followed by WARN_ON.

Adding a common lockdep interface will help reduce the duplication of this
logic in the rest of the kernel.

Add lockdep_assert_not_held() to be used in these functions to detect
incorrect calls while holding a lock.

lockdep_assert_not_held() provides the opposite functionality of
lockdep_assert_held() which is used to assert calls that require
holding a specific lock.

The need for lockdep_assert_not_held() came up in a discussion on
ath10k patch. ath10k_drain_tx() and i915_vma_pin_ww() are examples
of functions that can use lockdep_assert_not_held().

Link: https://lore.kernel.org/linux-wireless/871rdmu9z9.fsf@codeaurora.org/
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 include/linux/lockdep.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Peter Zijlstra Feb. 14, 2021, 5:53 p.m. UTC | #1
On Fri, Feb 12, 2021 at 04:28:42PM -0700, Shuah Khan wrote:

> +#define lockdep_assert_not_held(l)	do {			\

> +		WARN_ON(debug_locks && lockdep_is_held(l));	\

> +	} while (0)

> +


This thing isn't as straight forward as you might think, but it'll
mostly work.

Notably this thing will misfire when lockdep_off() is employed. It
certainyl needs a comment to explain the subtleties.
Peter Zijlstra Feb. 15, 2021, 10:44 a.m. UTC | #2
On Sun, Feb 14, 2021 at 06:53:01PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 12, 2021 at 04:28:42PM -0700, Shuah Khan wrote:

> 

> > +#define lockdep_assert_not_held(l)	do {			\

> > +		WARN_ON(debug_locks && lockdep_is_held(l));	\

> > +	} while (0)

> > +

> 

> This thing isn't as straight forward as you might think, but it'll

> mostly work.

> 

> Notably this thing will misfire when lockdep_off() is employed. It

> certainyl needs a comment to explain the subtleties.


I think something like so will work, but please double check.

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index b9e9adec73e8..c8b0d292bf8e 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
 
 #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
 
-#define lockdep_assert_held(l)	do {				\
-		WARN_ON(debug_locks && !lockdep_is_held(l));	\
+#define lockdep_assert_held(l)	do {					\
+		WARN_ON(debug_locks && lockdep_is_held(l) == 0));	\
 	} while (0)
 
-#define lockdep_assert_held_write(l)	do {			\
+#define lockdep_assert_not_held(l)	do {				\
+		WARN_ON(debug_locks && lockdep_is_held(l) == 1));	\
+	} while (0)
+
+#define lockdep_assert_held_write(l)	do {				\
 		WARN_ON(debug_locks && !lockdep_is_held_type(l, 0));	\
 	} while (0)
 
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index c1418b47f625..983ba206f7b2 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5467,7 +5467,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)
 	int ret = 0;
 
 	if (unlikely(!lockdep_enabled()))
-		return 1; /* avoid false negative lockdep_assert_held() */
+		return -1; /* avoid false negative lockdep_assert_held() */
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
Johannes Berg Feb. 15, 2021, 1:12 p.m. UTC | #3
On Mon, 2021-02-15 at 11:44 +0100, Peter Zijlstra wrote:
> 

> I think something like so will work, but please double check.


Yeah, that looks better.

> +++ b/include/linux/lockdep.h

> @@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);

>  

>  #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)

>  

> -#define lockdep_assert_held(l)	do {				\

> -		WARN_ON(debug_locks && !lockdep_is_held(l));	\

> +#define lockdep_assert_held(l)	do {					\

> +		WARN_ON(debug_locks && lockdep_is_held(l) == 0));	\

>  	} while (0)


That doesn't really need to change? It's the same.

> -#define lockdep_assert_held_write(l)	do {			\

> +#define lockdep_assert_not_held(l)	do {				\

> +		WARN_ON(debug_locks && lockdep_is_held(l) == 1));	\

> +	} while (0)

> +

> +#define lockdep_assert_held_write(l)	do {				\

>  		WARN_ON(debug_locks && !lockdep_is_held_type(l, 0));	\

>  	} while (0)

>  

> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c

> index c1418b47f625..983ba206f7b2 100644

> --- a/kernel/locking/lockdep.

> +++ b/kernel/locking/lockdep.c

> @@ -5467,7 +5467,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)

>  	int ret = 0;

>  

>  	if (unlikely(!lockdep_enabled()))

> -		return 1; /* avoid false negative lockdep_assert_held() */

> +		return -1; /* avoid false negative lockdep_assert_held() */


Maybe add lockdep_assert_not_held() to the comment, to explain the -1
(vs non-zero)?

johannes
Peter Zijlstra Feb. 15, 2021, 4:04 p.m. UTC | #4
On Mon, Feb 15, 2021 at 02:12:30PM +0100, Johannes Berg wrote:
> On Mon, 2021-02-15 at 11:44 +0100, Peter Zijlstra wrote:
> > 
> > I think something like so will work, but please double check.
> 
> Yeah, that looks better.
> 
> > +++ b/include/linux/lockdep.h
> > @@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
> >  
> >  #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
> >  
> > -#define lockdep_assert_held(l)	do {				\
> > -		WARN_ON(debug_locks && !lockdep_is_held(l));	\
> > +#define lockdep_assert_held(l)	do {					\
> > +		WARN_ON(debug_locks && lockdep_is_held(l) == 0));	\
> >  	} while (0)
> 
> That doesn't really need to change? It's the same.

Correct, but I found it more symmetric vs the not implementation below.

> > -#define lockdep_assert_held_write(l)	do {			\
> > +#define lockdep_assert_not_held(l)	do {				\
> > +		WARN_ON(debug_locks && lockdep_is_held(l) == 1));	\
> > +	} while (0)
> > +
> > +#define lockdep_assert_held_write(l)	do {				\
> >  		WARN_ON(debug_locks && !lockdep_is_held_type(l, 0));	\
> >  	} while (0)
> >  
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index c1418b47f625..983ba206f7b2 100644
> > --- a/kernel/locking/lockdep.
> > +++ b/kernel/locking/lockdep.c
> > @@ -5467,7 +5467,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)
> >  	int ret = 0;
> >  
> >  	if (unlikely(!lockdep_enabled()))
> > -		return 1; /* avoid false negative lockdep_assert_held() */
> > +		return -1; /* avoid false negative lockdep_assert_held() */
> 
> Maybe add lockdep_assert_not_held() to the comment, to explain the -1
> (vs non-zero)?

Yeah, or frob a '*' in there.
Johannes Berg Feb. 15, 2021, 4:10 p.m. UTC | #5
On Mon, 2021-02-15 at 17:04 +0100, Peter Zijlstra wrote:
> On Mon, Feb 15, 2021 at 02:12:30PM +0100, Johannes Berg wrote:
> > On Mon, 2021-02-15 at 11:44 +0100, Peter Zijlstra wrote:
> > > I think something like so will work, but please double check.
> > 
> > Yeah, that looks better.
> > 
> > > +++ b/include/linux/lockdep.h
> > > @@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
> > >  
> > >  #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
> > >  
> > > -#define lockdep_assert_held(l)	do {				\
> > > -		WARN_ON(debug_locks && !lockdep_is_held(l));	\
> > > +#define lockdep_assert_held(l)	do {					\
> > > +		WARN_ON(debug_locks && lockdep_is_held(l) == 0));	\
> > >  	} while (0)
> > 
> > That doesn't really need to change? It's the same.
> 
> Correct, but I found it more symmetric vs the not implementation below.

Fair enough. One might argue that you should have an

enum lockdep_lock_state {
	LOCK_STATE_NOT_HELD, /* 0 now */
	LOCK_STATE_HELD, /* 1 now */
	LOCK_STATE_UNKNOWN, /* -1 with your patch but might as well be 2 */
};

:)

johannes
Shuah Khan Feb. 22, 2021, 8:51 p.m. UTC | #6
On 2/15/21 9:10 AM, Johannes Berg wrote:
> On Mon, 2021-02-15 at 17:04 +0100, Peter Zijlstra wrote:
>> On Mon, Feb 15, 2021 at 02:12:30PM +0100, Johannes Berg wrote:
>>> On Mon, 2021-02-15 at 11:44 +0100, Peter Zijlstra wrote:
>>>> I think something like so will work, but please double check.
>>>
>>> Yeah, that looks better.
>>>
>>>> +++ b/include/linux/lockdep.h
>>>> @@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
>>>>   
>>>>   #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
>>>>   
>>>> -#define lockdep_assert_held(l)	do {				\
>>>> -		WARN_ON(debug_locks && !lockdep_is_held(l));	\
>>>> +#define lockdep_assert_held(l)	do {					\
>>>> +		WARN_ON(debug_locks && lockdep_is_held(l) == 0));	\
>>>>   	} while (0)
>>>
>>> That doesn't really need to change? It's the same.
>>
>> Correct, but I found it more symmetric vs the not implementation below.
> 
> Fair enough. One might argue that you should have an
> 
> enum lockdep_lock_state {
> 	LOCK_STATE_NOT_HELD, /* 0 now */
> 	LOCK_STATE_HELD, /* 1 now */
> 	LOCK_STATE_UNKNOWN, /* -1 with your patch but might as well be 2 */
> };
> 
> :)
> 


Thank you both. Picking this back up. Will send v2 incorporating
your comments and suggestions.

thanks,
-- Shuah
diff mbox series

Patch

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index b9e9adec73e8..567e3a1a27ce 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -294,6 +294,10 @@  extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
 
 #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
 
+#define lockdep_assert_not_held(l)	do {			\
+		WARN_ON(debug_locks && lockdep_is_held(l));	\
+	} while (0)
+
 #define lockdep_assert_held(l)	do {				\
 		WARN_ON(debug_locks && !lockdep_is_held(l));	\
 	} while (0)
@@ -383,8 +387,9 @@  extern int lock_is_held(const void *);
 extern int lockdep_is_held(const void *);
 #define lockdep_is_held_type(l, r)		(1)
 
+#define lockdep_assert_not_held(l)		do { (void)(l); } while (0)
 #define lockdep_assert_held(l)			do { (void)(l); } while (0)
-#define lockdep_assert_held_write(l)	do { (void)(l); } while (0)
+#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)