* [PATCH -tip] locking/rwsem: Check for operations on an uninitialized rwsem
@ 2019-07-29 4:47 Davidlohr Bueso
2019-07-29 10:38 ` Peter Zijlstra
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Davidlohr Bueso @ 2019-07-29 4:47 UTC (permalink / raw)
To: mingo, peterz; +Cc: longman, linux-kernel, Davidlohr Bueso, Davidlohr Bueso
Currently rwsems is the only locking primitive that lacks this
debug feature. Add it under CONFIG_DEBUG_RWSEMS and do the magic
checking in the locking fastpath (trylock) operation such that
we cover all cases. The unlocking part is pretty straightforward.
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
include/linux/rwsem.h | 10 ++++++++++
kernel/locking/rwsem.c | 22 ++++++++++++++++++----
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 9d9c663987d8..00d6054687dd 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -45,6 +45,9 @@ struct rw_semaphore {
#endif
raw_spinlock_t wait_lock;
struct list_head wait_list;
+#ifdef CONFIG_DEBUG_RWSEMS
+ void *magic;
+#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
struct lockdep_map dep_map;
#endif
@@ -73,6 +76,12 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
# define __RWSEM_DEP_MAP_INIT(lockname)
#endif
+#ifdef CONFIG_DEBUG_RWSEMS
+# define __DEBUG_RWSEM_INITIALIZER(lockname) , .magic = &lockname
+#else
+# define __DEBUG_RWSEM_INITIALIZER(lockname)
+#endif
+
#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
#define __RWSEM_OPT_INIT(lockname) , .osq = OSQ_LOCK_UNLOCKED
#else
@@ -85,6 +94,7 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
.wait_list = LIST_HEAD_INIT((name).wait_list), \
.wait_lock = __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock) \
__RWSEM_OPT_INIT(name) \
+ __DEBUG_RWSEM_INITIALIZER(name) \
__RWSEM_DEP_MAP_INIT(name) }
#define DECLARE_RWSEM(name) \
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 37524a47f002..ab392ec51252 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -105,8 +105,9 @@
#ifdef CONFIG_DEBUG_RWSEMS
# define DEBUG_RWSEMS_WARN_ON(c, sem) do { \
if (!debug_locks_silent && \
- WARN_ONCE(c, "DEBUG_RWSEMS_WARN_ON(%s): count = 0x%lx, owner = 0x%lx, curr 0x%lx, list %sempty\n",\
+ WARN_ONCE(c, "DEBUG_RWSEMS_WARN_ON(%s): count = 0x%lx, magic = 0x%lx, owner = 0x%lx, curr 0x%lx, list %sempty\n",\
#c, atomic_long_read(&(sem)->count), \
+ (unsigned long) sem->magic, \
atomic_long_read(&(sem)->owner), (long)current, \
list_empty(&(sem)->wait_list) ? "" : "not ")) \
debug_locks_off(); \
@@ -329,6 +330,9 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
*/
debug_check_no_locks_freed((void *)sem, sizeof(*sem));
lockdep_init_map(&sem->dep_map, name, key, 0);
+#endif
+#ifdef CONFIG_DEBUG_RWSEMS
+ sem->magic = sem;
#endif
atomic_long_set(&sem->count, RWSEM_UNLOCKED_VALUE);
raw_spin_lock_init(&sem->wait_lock);
@@ -1322,11 +1326,14 @@ static inline int __down_read_killable(struct rw_semaphore *sem)
static inline int __down_read_trylock(struct rw_semaphore *sem)
{
+ long tmp;
+
+ DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
+
/*
* Optimize for the case when the rwsem is not locked at all.
*/
- long tmp = RWSEM_UNLOCKED_VALUE;
-
+ tmp = RWSEM_UNLOCKED_VALUE;
do {
if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
tmp + RWSEM_READER_BIAS)) {
@@ -1367,8 +1374,11 @@ static inline int __down_write_killable(struct rw_semaphore *sem)
static inline int __down_write_trylock(struct rw_semaphore *sem)
{
- long tmp = RWSEM_UNLOCKED_VALUE;
+ long tmp;
+ DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
+
+ tmp = RWSEM_UNLOCKED_VALUE;
if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
RWSEM_WRITER_LOCKED)) {
rwsem_set_owner(sem);
@@ -1384,7 +1394,9 @@ inline void __up_read(struct rw_semaphore *sem)
{
long tmp;
+ DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+
rwsem_clear_reader_owned(sem);
tmp = atomic_long_add_return_release(-RWSEM_READER_BIAS, &sem->count);
DEBUG_RWSEMS_WARN_ON(tmp < 0, sem);
@@ -1402,12 +1414,14 @@ static inline void __up_write(struct rw_semaphore *sem)
{
long tmp;
+ DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
/*
* sem->owner may differ from current if the ownership is transferred
* to an anonymous writer by setting the RWSEM_NONSPINNABLE bits.
*/
DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) &&
!rwsem_test_oflags(sem, RWSEM_NONSPINNABLE), sem);
+
rwsem_clear_owner(sem);
tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
if (unlikely(tmp & RWSEM_FLAG_WAITERS))
--
2.16.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH -tip] locking/rwsem: Check for operations on an uninitialized rwsem
2019-07-29 4:47 [PATCH -tip] locking/rwsem: Check for operations on an uninitialized rwsem Davidlohr Bueso
@ 2019-07-29 10:38 ` Peter Zijlstra
2019-07-29 14:36 ` Waiman Long
2019-08-06 13:05 ` [tip:locking/core] " tip-bot for Davidlohr Bueso
2 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2019-07-29 10:38 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: mingo, longman, linux-kernel, Davidlohr Bueso
On Sun, Jul 28, 2019 at 09:47:35PM -0700, Davidlohr Bueso wrote:
> Currently rwsems is the only locking primitive that lacks this
> debug feature. Add it under CONFIG_DEBUG_RWSEMS and do the magic
> checking in the locking fastpath (trylock) operation such that
> we cover all cases. The unlocking part is pretty straightforward.
Thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH -tip] locking/rwsem: Check for operations on an uninitialized rwsem
2019-07-29 4:47 [PATCH -tip] locking/rwsem: Check for operations on an uninitialized rwsem Davidlohr Bueso
2019-07-29 10:38 ` Peter Zijlstra
@ 2019-07-29 14:36 ` Waiman Long
2019-08-06 13:05 ` [tip:locking/core] " tip-bot for Davidlohr Bueso
2 siblings, 0 replies; 4+ messages in thread
From: Waiman Long @ 2019-07-29 14:36 UTC (permalink / raw)
To: Davidlohr Bueso, mingo, peterz; +Cc: linux-kernel, Davidlohr Bueso
On 7/29/19 12:47 AM, Davidlohr Bueso wrote:
> Currently rwsems is the only locking primitive that lacks this
> debug feature. Add it under CONFIG_DEBUG_RWSEMS and do the magic
> checking in the locking fastpath (trylock) operation such that
> we cover all cases. The unlocking part is pretty straightforward.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> include/linux/rwsem.h | 10 ++++++++++
> kernel/locking/rwsem.c | 22 ++++++++++++++++++----
> 2 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
> index 9d9c663987d8..00d6054687dd 100644
> --- a/include/linux/rwsem.h
> +++ b/include/linux/rwsem.h
> @@ -45,6 +45,9 @@ struct rw_semaphore {
> #endif
> raw_spinlock_t wait_lock;
> struct list_head wait_list;
> +#ifdef CONFIG_DEBUG_RWSEMS
> + void *magic;
> +#endif
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> struct lockdep_map dep_map;
> #endif
> @@ -73,6 +76,12 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
> # define __RWSEM_DEP_MAP_INIT(lockname)
> #endif
>
> +#ifdef CONFIG_DEBUG_RWSEMS
> +# define __DEBUG_RWSEM_INITIALIZER(lockname) , .magic = &lockname
> +#else
> +# define __DEBUG_RWSEM_INITIALIZER(lockname)
> +#endif
> +
> #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
> #define __RWSEM_OPT_INIT(lockname) , .osq = OSQ_LOCK_UNLOCKED
> #else
> @@ -85,6 +94,7 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
> .wait_list = LIST_HEAD_INIT((name).wait_list), \
> .wait_lock = __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock) \
> __RWSEM_OPT_INIT(name) \
> + __DEBUG_RWSEM_INITIALIZER(name) \
> __RWSEM_DEP_MAP_INIT(name) }
>
> #define DECLARE_RWSEM(name) \
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index 37524a47f002..ab392ec51252 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -105,8 +105,9 @@
> #ifdef CONFIG_DEBUG_RWSEMS
> # define DEBUG_RWSEMS_WARN_ON(c, sem) do { \
> if (!debug_locks_silent && \
> - WARN_ONCE(c, "DEBUG_RWSEMS_WARN_ON(%s): count = 0x%lx, owner = 0x%lx, curr 0x%lx, list %sempty\n",\
> + WARN_ONCE(c, "DEBUG_RWSEMS_WARN_ON(%s): count = 0x%lx, magic = 0x%lx, owner = 0x%lx, curr 0x%lx, list %sempty\n",\
> #c, atomic_long_read(&(sem)->count), \
> + (unsigned long) sem->magic, \
> atomic_long_read(&(sem)->owner), (long)current, \
> list_empty(&(sem)->wait_list) ? "" : "not ")) \
> debug_locks_off(); \
> @@ -329,6 +330,9 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
> */
> debug_check_no_locks_freed((void *)sem, sizeof(*sem));
> lockdep_init_map(&sem->dep_map, name, key, 0);
> +#endif
> +#ifdef CONFIG_DEBUG_RWSEMS
> + sem->magic = sem;
> #endif
> atomic_long_set(&sem->count, RWSEM_UNLOCKED_VALUE);
> raw_spin_lock_init(&sem->wait_lock);
> @@ -1322,11 +1326,14 @@ static inline int __down_read_killable(struct rw_semaphore *sem)
>
> static inline int __down_read_trylock(struct rw_semaphore *sem)
> {
> + long tmp;
> +
> + DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
> +
> /*
> * Optimize for the case when the rwsem is not locked at all.
> */
> - long tmp = RWSEM_UNLOCKED_VALUE;
> -
> + tmp = RWSEM_UNLOCKED_VALUE;
> do {
> if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
> tmp + RWSEM_READER_BIAS)) {
> @@ -1367,8 +1374,11 @@ static inline int __down_write_killable(struct rw_semaphore *sem)
>
> static inline int __down_write_trylock(struct rw_semaphore *sem)
> {
> - long tmp = RWSEM_UNLOCKED_VALUE;
> + long tmp;
>
> + DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
> +
> + tmp = RWSEM_UNLOCKED_VALUE;
> if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
> RWSEM_WRITER_LOCKED)) {
> rwsem_set_owner(sem);
> @@ -1384,7 +1394,9 @@ inline void __up_read(struct rw_semaphore *sem)
> {
> long tmp;
>
> + DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
> DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
> +
> rwsem_clear_reader_owned(sem);
> tmp = atomic_long_add_return_release(-RWSEM_READER_BIAS, &sem->count);
> DEBUG_RWSEMS_WARN_ON(tmp < 0, sem);
> @@ -1402,12 +1414,14 @@ static inline void __up_write(struct rw_semaphore *sem)
> {
> long tmp;
>
> + DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
> /*
> * sem->owner may differ from current if the ownership is transferred
> * to an anonymous writer by setting the RWSEM_NONSPINNABLE bits.
> */
> DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) &&
> !rwsem_test_oflags(sem, RWSEM_NONSPINNABLE), sem);
> +
> rwsem_clear_owner(sem);
> tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
> if (unlikely(tmp & RWSEM_FLAG_WAITERS))
Acked-by: Waiman Long <longman@redhat.com>
Thanks for the patch.
-Longman
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tip:locking/core] locking/rwsem: Check for operations on an uninitialized rwsem
2019-07-29 4:47 [PATCH -tip] locking/rwsem: Check for operations on an uninitialized rwsem Davidlohr Bueso
2019-07-29 10:38 ` Peter Zijlstra
2019-07-29 14:36 ` Waiman Long
@ 2019-08-06 13:05 ` tip-bot for Davidlohr Bueso
2 siblings, 0 replies; 4+ messages in thread
From: tip-bot for Davidlohr Bueso @ 2019-08-06 13:05 UTC (permalink / raw)
To: linux-tip-commits
Cc: hpa, dave, linux-kernel, mingo, longman, tglx, peterz, dbueso
Commit-ID: fce45cd41101f1a9620267146b21f09b3454d8db
Gitweb: https://git.kernel.org/tip/fce45cd41101f1a9620267146b21f09b3454d8db
Author: Davidlohr Bueso <dave@stgolabs.net>
AuthorDate: Sun, 28 Jul 2019 21:47:35 -0700
Committer: Peter Zijlstra <peterz@infradead.org>
CommitDate: Tue, 6 Aug 2019 12:49:15 +0200
locking/rwsem: Check for operations on an uninitialized rwsem
Currently rwsems is the only locking primitive that lacks this
debug feature. Add it under CONFIG_DEBUG_RWSEMS and do the magic
checking in the locking fastpath (trylock) operation such that
we cover all cases. The unlocking part is pretty straightforward.
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Waiman Long <longman@redhat.com>
Cc: mingo@kernel.org
Cc: Davidlohr Bueso <dave@stgolabs.net>
Link: https://lkml.kernel.org/r/20190729044735.9632-1-dave@stgolabs.net
---
include/linux/rwsem.h | 10 ++++++++++
kernel/locking/rwsem.c | 22 ++++++++++++++++++----
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 9d9c663987d8..00d6054687dd 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -45,6 +45,9 @@ struct rw_semaphore {
#endif
raw_spinlock_t wait_lock;
struct list_head wait_list;
+#ifdef CONFIG_DEBUG_RWSEMS
+ void *magic;
+#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
struct lockdep_map dep_map;
#endif
@@ -73,6 +76,12 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
# define __RWSEM_DEP_MAP_INIT(lockname)
#endif
+#ifdef CONFIG_DEBUG_RWSEMS
+# define __DEBUG_RWSEM_INITIALIZER(lockname) , .magic = &lockname
+#else
+# define __DEBUG_RWSEM_INITIALIZER(lockname)
+#endif
+
#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
#define __RWSEM_OPT_INIT(lockname) , .osq = OSQ_LOCK_UNLOCKED
#else
@@ -85,6 +94,7 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
.wait_list = LIST_HEAD_INIT((name).wait_list), \
.wait_lock = __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock) \
__RWSEM_OPT_INIT(name) \
+ __DEBUG_RWSEM_INITIALIZER(name) \
__RWSEM_DEP_MAP_INIT(name) }
#define DECLARE_RWSEM(name) \
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 354238a08b7a..eef04551eae7 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -105,8 +105,9 @@
#ifdef CONFIG_DEBUG_RWSEMS
# define DEBUG_RWSEMS_WARN_ON(c, sem) do { \
if (!debug_locks_silent && \
- WARN_ONCE(c, "DEBUG_RWSEMS_WARN_ON(%s): count = 0x%lx, owner = 0x%lx, curr 0x%lx, list %sempty\n",\
+ WARN_ONCE(c, "DEBUG_RWSEMS_WARN_ON(%s): count = 0x%lx, magic = 0x%lx, owner = 0x%lx, curr 0x%lx, list %sempty\n",\
#c, atomic_long_read(&(sem)->count), \
+ (unsigned long) sem->magic, \
atomic_long_read(&(sem)->owner), (long)current, \
list_empty(&(sem)->wait_list) ? "" : "not ")) \
debug_locks_off(); \
@@ -329,6 +330,9 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
*/
debug_check_no_locks_freed((void *)sem, sizeof(*sem));
lockdep_init_map(&sem->dep_map, name, key, 0);
+#endif
+#ifdef CONFIG_DEBUG_RWSEMS
+ sem->magic = sem;
#endif
atomic_long_set(&sem->count, RWSEM_UNLOCKED_VALUE);
raw_spin_lock_init(&sem->wait_lock);
@@ -1358,11 +1362,14 @@ static inline int __down_read_killable(struct rw_semaphore *sem)
static inline int __down_read_trylock(struct rw_semaphore *sem)
{
+ long tmp;
+
+ DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
+
/*
* Optimize for the case when the rwsem is not locked at all.
*/
- long tmp = RWSEM_UNLOCKED_VALUE;
-
+ tmp = RWSEM_UNLOCKED_VALUE;
do {
if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
tmp + RWSEM_READER_BIAS)) {
@@ -1403,8 +1410,11 @@ static inline int __down_write_killable(struct rw_semaphore *sem)
static inline int __down_write_trylock(struct rw_semaphore *sem)
{
- long tmp = RWSEM_UNLOCKED_VALUE;
+ long tmp;
+ DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
+
+ tmp = RWSEM_UNLOCKED_VALUE;
if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
RWSEM_WRITER_LOCKED)) {
rwsem_set_owner(sem);
@@ -1420,7 +1430,9 @@ inline void __up_read(struct rw_semaphore *sem)
{
long tmp;
+ DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+
rwsem_clear_reader_owned(sem);
tmp = atomic_long_add_return_release(-RWSEM_READER_BIAS, &sem->count);
DEBUG_RWSEMS_WARN_ON(tmp < 0, sem);
@@ -1438,12 +1450,14 @@ static inline void __up_write(struct rw_semaphore *sem)
{
long tmp;
+ DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
/*
* sem->owner may differ from current if the ownership is transferred
* to an anonymous writer by setting the RWSEM_NONSPINNABLE bits.
*/
DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) &&
!rwsem_test_oflags(sem, RWSEM_NONSPINNABLE), sem);
+
rwsem_clear_owner(sem);
tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
if (unlikely(tmp & RWSEM_FLAG_WAITERS))
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-06 13:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-29 4:47 [PATCH -tip] locking/rwsem: Check for operations on an uninitialized rwsem Davidlohr Bueso
2019-07-29 10:38 ` Peter Zijlstra
2019-07-29 14:36 ` Waiman Long
2019-08-06 13:05 ` [tip:locking/core] " tip-bot for Davidlohr Bueso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.