linux-kernel - Re: [PATCH] locking/lockdep: Fix lockdep_init_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Jun 2022 10:57:11 -0400
From:   Waiman Long <longman@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>, mingo@...hat.com,
        will@...nel.org, boqun.feng@...il.com
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH] locking/lockdep: Fix lockdep_init_map_*() confusion

On 6/17/22 09:39, Peter Zijlstra wrote:
> Commit dfd5e3f5fe27 ("locking/lockdep: Mark local_lock_t") added yet
> another lockdep_init_map_*() variant, but forgot to update all the
> existing users of the most complicated version.
>
> This could lead to a loss of lock_type and hence an incorrect report.
> Given the relative rarity of both local_lock and these annotations,
> this is unlikely to happen in practise, still, best fix things.
>
> Fixes: dfd5e3f5fe27 ("locking/lockdep: Mark local_lock_t")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>   include/linux/lockdep.h  |   30 +++++++++++++++++-------------
>   kernel/locking/lockdep.c |    7 ++++---
>   2 files changed, 21 insertions(+), 16 deletions(-)
>
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -188,7 +188,7 @@ static inline void
>   lockdep_init_map_waits(struct lockdep_map *lock, const char *name,
>   		       struct lock_class_key *key, int subclass, u8 inner, u8 outer)
>   {
> -	lockdep_init_map_type(lock, name, key, subclass, inner, LD_WAIT_INV, LD_LOCK_NORMAL);
> +	lockdep_init_map_type(lock, name, key, subclass, inner, outer, LD_LOCK_NORMAL);
>   }
>   
>   static inline void
> @@ -211,24 +211,28 @@ static inline void lockdep_init_map(stru
>    * or they are too narrow (they suffer from a false class-split):
>    */
>   #define lockdep_set_class(lock, key)				\
> -	lockdep_init_map_waits(&(lock)->dep_map, #key, key, 0,	\
> -			       (lock)->dep_map.wait_type_inner,	\
> -			       (lock)->dep_map.wait_type_outer)
> +	lockdep_init_map_type(&(lock)->dep_map, #key, key, 0,	\
> +			      (lock)->dep_map.wait_type_inner,	\
> +			      (lock)->dep_map.wait_type_outer,	\
> +			      (lock)->dep_map.lock_type)
>   
>   #define lockdep_set_class_and_name(lock, key, name)		\
> -	lockdep_init_map_waits(&(lock)->dep_map, name, key, 0,	\
> -			       (lock)->dep_map.wait_type_inner,	\
> -			       (lock)->dep_map.wait_type_outer)
> +	lockdep_init_map_type(&(lock)->dep_map, name, key, 0,	\
> +			      (lock)->dep_map.wait_type_inner,	\
> +			      (lock)->dep_map.wait_type_outer,	\
> +			      (lock)->dep_map.lock_type)
>   
>   #define lockdep_set_class_and_subclass(lock, key, sub)		\
> -	lockdep_init_map_waits(&(lock)->dep_map, #key, key, sub,\
> -			       (lock)->dep_map.wait_type_inner,	\
> -			       (lock)->dep_map.wait_type_outer)
> +	lockdep_init_map_type(&(lock)->dep_map, #key, key, sub,	\
> +			      (lock)->dep_map.wait_type_inner,	\
> +			      (lock)->dep_map.wait_type_outer,	\
> +			      (lock)->dep_map.lock_type)
>   
>   #define lockdep_set_subclass(lock, sub)					\
> -	lockdep_init_map_waits(&(lock)->dep_map, #lock, (lock)->dep_map.key, sub,\
> -			       (lock)->dep_map.wait_type_inner,		\
> -			       (lock)->dep_map.wait_type_outer)
> +	lockdep_init_map_type(&(lock)->dep_map, #lock, (lock)->dep_map.key, sub,\
> +			      (lock)->dep_map.wait_type_inner,		\
> +			      (lock)->dep_map.wait_type_outer,		\
> +			      (lock)->dep_map.lock_type)
>   
>   #define lockdep_set_novalidate_class(lock) \
>   	lockdep_set_class_and_name(lock, &__lockdep_no_validate__, #lock)
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -5238,9 +5238,10 @@ __lock_set_class(struct lockdep_map *loc
>   		return 0;
>   	}
>   
> -	lockdep_init_map_waits(lock, name, key, 0,
> -			       lock->wait_type_inner,
> -			       lock->wait_type_outer);
> +	lockdep_init_map_type(lock, name, key, 0,
> +			      lock->wait_type_inner,
> +			      lock->wait_type_outer,
> +			      lock->lock_type);
>   	class = register_lock_class(lock, subclass, 0);
>   	hlock->class_idx = class - lock_classes;
>   
>
You have almost eliminated all usage of lockdep_init_map_waits() except 
in lockdep_init_map_wait(). Should we just kill lockdep_init_map_waits() 
and make lockdep_init_map_wait() call lockdep_init_map_type() directly?

Cheers,
Longman