[PATCH v2] locking/rwsem: Add DEBUG_RWSEMS to look for lock/unlock mismatches

[PATCH v2] locking/rwsem: Add DEBUG_RWSEMS to look for lock/unlock mismatches

[Waiman Long]

For a rwsem, locking can either be exclusive or shared. The corresponding
exclusive or shared unlock must be used. Otherwise, the protected data
structures may get corrupted or the lock may be in an inconsistent state.

In order to detect such anomaly, a new configuration option DEBUG_RWSEMS
is added which can be enabled to look for such mismatches and print
warnings that that happens.

Signed-off-by: Waiman Long <longman@redhat.com>

 v2: - Fix typo

---
 kernel/locking/rwsem.c | 4 ++++
 kernel/locking/rwsem.h | 8 +++++++-
 lib/Kconfig.debug      | 7 +++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index f549c55..30465a2 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -117,6 +117,7 @@ int down_write_trylock(struct rw_semaphore *sem)
 void up_read(struct rw_semaphore *sem)
 {
 	rwsem_release(&sem->dep_map, 1, _RET_IP_);
+	DEBUG_RWSEMS_WARN_ON(sem->owner != RWSEM_READER_OWNED);
 
 	__up_read(sem);
 }
@@ -129,6 +130,7 @@ void up_read(struct rw_semaphore *sem)
 void up_write(struct rw_semaphore *sem)
 {
 	rwsem_release(&sem->dep_map, 1, _RET_IP_);
+	DEBUG_RWSEMS_WARN_ON(sem->owner != current);
 
 	rwsem_clear_owner(sem);
 	__up_write(sem);
@@ -142,6 +144,7 @@ void up_write(struct rw_semaphore *sem)
 void downgrade_write(struct rw_semaphore *sem)
 {
 	lock_downgrade(&sem->dep_map, _RET_IP_);
+	DEBUG_RWSEMS_WARN_ON(sem->owner != current);
 
 	rwsem_set_reader_owned(sem);
 	__downgrade_write(sem);
@@ -211,6 +214,7 @@ int __sched down_write_killable_nested(struct rw_semaphore *sem, int subclass)
 
 void up_read_non_owner(struct rw_semaphore *sem)
 {
+	DEBUG_RWSEMS_WARN_ON(sem->owner != RWSEM_READER_OWNED);
 	__up_read(sem);
 }
 
diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h
index a883b8f..563a7bc 100644
--- a/kernel/locking/rwsem.h
+++ b/kernel/locking/rwsem.h
@@ -16,6 +16,12 @@
  */
 #define RWSEM_READER_OWNED	((struct task_struct *)1UL)
 
+#ifdef CONFIG_DEBUG_RWSEMS
+#define DEBUG_RWSEMS_WARN_ON(c)	DEBUG_LOCKS_WARN_ON(c)
+#else
+#define DEBUG_RWSEMS_WARN_ON(c)
+#endif
+
 #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 /*
  * All writes to owner are protected by WRITE_ONCE() to make sure that
@@ -41,7 +47,7 @@ static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
 	 * do a write to the rwsem cacheline when it is really necessary
 	 * to minimize cacheline contention.
 	 */
-	if (sem->owner != RWSEM_READER_OWNED)
+	if (READ_ONCE(sem->owner) != RWSEM_READER_OWNED)
 		WRITE_ONCE(sem->owner, RWSEM_READER_OWNED);
 }
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 64155e3..0958192 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1075,6 +1075,13 @@ config DEBUG_WW_MUTEX_SLOWPATH
 	 even a debug kernel.  If you are a driver writer, enable it.  If
 	 you are a distro, do not.
 
+config DEBUG_RWSEMS
+	bool "RW Semaphore debugging: basic checks"
+	depends on DEBUG_KERNEL && RWSEM_SPIN_ON_OWNER
+	help
+	  This feature allows mismatched rw semaphore locks and unlocks
+	  to be detected and reported.
+
 config DEBUG_LOCK_ALLOC
 	bool "Lock debugging: detect incorrect freeing of live locks"
 	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
-- 
1.8.3.1

Re: [PATCH v2] locking/rwsem: Add DEBUG_RWSEMS to look for lock/unlock mismatches

[Ingo Molnar]

* Waiman Long <longman@redhat.com> wrote:

> For a rwsem, locking can either be exclusive or shared. The corresponding
> exclusive or shared unlock must be used. Otherwise, the protected data
> structures may get corrupted or the lock may be in an inconsistent state.
> 
> In order to detect such anomaly, a new configuration option DEBUG_RWSEMS
> is added which can be enabled to look for such mismatches and print
> warnings that that happens.

> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 64155e3..0958192 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1075,6 +1075,13 @@ config DEBUG_WW_MUTEX_SLOWPATH
>  	 even a debug kernel.  If you are a driver writer, enable it.  If
>  	 you are a distro, do not.
>  
> +config DEBUG_RWSEMS
> +	bool "RW Semaphore debugging: basic checks"
> +	depends on DEBUG_KERNEL && RWSEM_SPIN_ON_OWNER
> +	help
> +	  This feature allows mismatched rw semaphore locks and unlocks
> +	  to be detected and reported.
> +

Makes sense - but this should also be integrated into the rest of lock debugging 
Kconfig hierarchy similar to DEBUG_MUTEXES: i.e. DEBUG_LOCK_ALLOC, PROVE_LOCKING, 
etc. should select this new lock debugging option as well.

People generally are not supposed to know and configure the finer details, 
CONFIG_LOCK_DEBUGGING=y is a one-stop-shop in this regard.

Thanks,

	Ingo

Re: [PATCH v2] locking/rwsem: Add DEBUG_RWSEMS to look for lock/unlock mismatches

[Waiman Long]

On 03/27/2018 02:06 AM, Ingo Molnar wrote:
> * Waiman Long <longman@redhat.com> wrote:
>
>> For a rwsem, locking can either be exclusive or shared. The corresponding
>> exclusive or shared unlock must be used. Otherwise, the protected data
>> structures may get corrupted or the lock may be in an inconsistent state.
>>
>> In order to detect such anomaly, a new configuration option DEBUG_RWSEMS
>> is added which can be enabled to look for such mismatches and print
>> warnings that that happens.
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index 64155e3..0958192 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -1075,6 +1075,13 @@ config DEBUG_WW_MUTEX_SLOWPATH
>>  	 even a debug kernel.  If you are a driver writer, enable it.  If
>>  	 you are a distro, do not.
>>  
>> +config DEBUG_RWSEMS
>> +	bool "RW Semaphore debugging: basic checks"
>> +	depends on DEBUG_KERNEL && RWSEM_SPIN_ON_OWNER
>> +	help
>> +	  This feature allows mismatched rw semaphore locks and unlocks
>> +	  to be detected and reported.
>> +
> Makes sense - but this should also be integrated into the rest of lock debugging 
> Kconfig hierarchy similar to DEBUG_MUTEXES: i.e. DEBUG_LOCK_ALLOC, PROVE_LOCKING, 
> etc. should select this new lock debugging option as well.
>
> People generally are not supposed to know and configure the finer details, 
> CONFIG_LOCK_DEBUGGING=y is a one-stop-shop in this regard.

I can add a patch to rework lock debugging configuration code. Do you
want to hide the individual config options but still allow them to be
enabled manually? Alternatively we can also just add a master lock
debugging option to select them all.

Cheers,
Longman

Re: [PATCH v2] locking/rwsem: Add DEBUG_RWSEMS to look for lock/unlock mismatches

[Davidlohr Bueso]

On Tue, 27 Mar 2018, Waiman Long wrote:
>I can add a patch to rework lock debugging configuration code. D

Please in the same patch, just a v3.

Thanks
Davidlohr

Re: [PATCH v2] locking/rwsem: Add DEBUG_RWSEMS to look for lock/unlock mismatches

[Ingo Molnar]

* Waiman Long <longman@redhat.com> wrote:

> On 03/27/2018 02:06 AM, Ingo Molnar wrote:
> > * Waiman Long <longman@redhat.com> wrote:
> >
> >> For a rwsem, locking can either be exclusive or shared. The corresponding
> >> exclusive or shared unlock must be used. Otherwise, the protected data
> >> structures may get corrupted or the lock may be in an inconsistent state.
> >>
> >> In order to detect such anomaly, a new configuration option DEBUG_RWSEMS
> >> is added which can be enabled to look for such mismatches and print
> >> warnings that that happens.
> >> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> >> index 64155e3..0958192 100644
> >> --- a/lib/Kconfig.debug
> >> +++ b/lib/Kconfig.debug
> >> @@ -1075,6 +1075,13 @@ config DEBUG_WW_MUTEX_SLOWPATH
> >>  	 even a debug kernel.  If you are a driver writer, enable it.  If
> >>  	 you are a distro, do not.
> >>  
> >> +config DEBUG_RWSEMS
> >> +	bool "RW Semaphore debugging: basic checks"
> >> +	depends on DEBUG_KERNEL && RWSEM_SPIN_ON_OWNER
> >> +	help
> >> +	  This feature allows mismatched rw semaphore locks and unlocks
> >> +	  to be detected and reported.
> >> +
> > Makes sense - but this should also be integrated into the rest of lock debugging 
> > Kconfig hierarchy similar to DEBUG_MUTEXES: i.e. DEBUG_LOCK_ALLOC, PROVE_LOCKING, 
> > etc. should select this new lock debugging option as well.
> >
> > People generally are not supposed to know and configure the finer details, 
> > CONFIG_LOCK_DEBUGGING=y is a one-stop-shop in this regard.
> 
> I can add a patch to rework lock debugging configuration code. Do you
> want to hide the individual config options but still allow them to be
> enabled manually? Alternatively we can also just add a master lock
> debugging option to select them all.

They should work like mutex debugging: it's auto-selected by PROVE_LOCKING, but 
can be enabled individually as well if PROVE_LOCKING is disabled.

Thanks,

	Ingo

Re: [PATCH v2] locking/rwsem: Add DEBUG_RWSEMS to look for lock/unlock mismatches

[Waiman Long]

On 03/27/2018 03:56 PM, Ingo Molnar wrote:
> * Waiman Long <longman@redhat.com> wrote:
>
>> On 03/27/2018 02:06 AM, Ingo Molnar wrote:
>>> * Waiman Long <longman@redhat.com> wrote:
>>>
>>>> For a rwsem, locking can either be exclusive or shared. The corresponding
>>>> exclusive or shared unlock must be used. Otherwise, the protected data
>>>> structures may get corrupted or the lock may be in an inconsistent state.
>>>>
>>>> In order to detect such anomaly, a new configuration option DEBUG_RWSEMS
>>>> is added which can be enabled to look for such mismatches and print
>>>> warnings that that happens.
>>>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>>>> index 64155e3..0958192 100644
>>>> --- a/lib/Kconfig.debug
>>>> +++ b/lib/Kconfig.debug
>>>> @@ -1075,6 +1075,13 @@ config DEBUG_WW_MUTEX_SLOWPATH
>>>>  	 even a debug kernel.  If you are a driver writer, enable it.  If
>>>>  	 you are a distro, do not.
>>>>  
>>>> +config DEBUG_RWSEMS
>>>> +	bool "RW Semaphore debugging: basic checks"
>>>> +	depends on DEBUG_KERNEL && RWSEM_SPIN_ON_OWNER
>>>> +	help
>>>> +	  This feature allows mismatched rw semaphore locks and unlocks
>>>> +	  to be detected and reported.
>>>> +
>>> Makes sense - but this should also be integrated into the rest of lock debugging 
>>> Kconfig hierarchy similar to DEBUG_MUTEXES: i.e. DEBUG_LOCK_ALLOC, PROVE_LOCKING, 
>>> etc. should select this new lock debugging option as well.
>>>
>>> People generally are not supposed to know and configure the finer details, 
>>> CONFIG_LOCK_DEBUGGING=y is a one-stop-shop in this regard.
>> I can add a patch to rework lock debugging configuration code. Do you
>> want to hide the individual config options but still allow them to be
>> enabled manually? Alternatively we can also just add a master lock
>> debugging option to select them all.
> They should work like mutex debugging: it's auto-selected by PROVE_LOCKING, but 
> can be enabled individually as well if PROVE_LOCKING is disabled.
Got it.

Thanks,
Longman