[PATCH 0/3] Lock warning cleanup

[PATCH 0/3] Lock warning cleanup

[Jules Irenge]

1. TIME subsytem : patch 1, an __acquires(timer) annotation is added. as
the function despite having a nested lock the outer one allows entry to
critical section only.
2. Within futex.c file or path 2, a __releases() annotation is added. as the
function releases the lock at exit.
3. MUTEX subsystem : patch 3, __acquires(lock) and __releases(lock) are
added to mutex_lock() and mutex_unlock() to fix issues raised in other
files. 

Jules Irenge (3):
  time: Add missing annotation to lock_hrtimer_base()
  futex: Add missing annotation for wake_futex_pi()
  mutex: Add missing annotations

 include/linux/mutex.h | 4 ++--
 kernel/futex.c        | 1 +
 kernel/time/hrtimer.c | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.24.1

[PATCH 2/3] futex: Add missing annotation for wake_futex_pi()

[Jules Irenge]

Sparse reports a warning at wake_futex_pi()

|warning: context imbalance in wake_futex_pi() - unexpected unlock
To fix this,
a __releases(&pi_state->pi_mutex.wait_lock) annotation is added
Given that wake_futex_pi() does actually call
raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock)
This not only fixes the warning
but also improves on the readability of the code.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/futex.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/futex.c b/kernel/futex.c
index 03c518e9747e..8bc288a7187f 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1549,6 +1549,7 @@ static void mark_wake_futex(struct wake_q_head *wake_q, struct futex_q *q)
  * Caller must hold a reference on @pi_state.
  */
 static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_pi_state *pi_state)
+	__releases(&pi_state->pi_mutex.wait_lock)
 {
 	u32 uninitialized_var(curval), newval;
 	struct task_struct *new_owner;
-- 
2.24.1

[PATCH 3/3] mutex: Add missing annotations

[Jules Irenge]

Sparse reports false warnings and hide real warnings
where mutex_lock() and mutex_unlock() are used within the kernel
An example is within the kernel cgroup files
where the below warnings are found
|warning: context imbalance in cgroup_lock_and_drain_offline()
| - wrong count at exit
|warning: context imbalance in cgroup_procs_write_finish()
|- wrong count at exit
|warning: context imbalance in cgroup_procs_write_start()
|- wrong count at exit.

To fix these,
an __acquires(lock) is added to mutex_lock() declaration
a __releases(lock) to mutex_unlock() declaration

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 include/linux/mutex.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index aca8f36dfac9..a8ab4029913e 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -162,7 +162,7 @@ do {									\
 } while (0)
 
 #else
-extern void mutex_lock(struct mutex *lock);
+extern void mutex_lock(struct mutex *lock) __acquires(lock);
 extern int __must_check mutex_lock_interruptible(struct mutex *lock);
 extern int __must_check mutex_lock_killable(struct mutex *lock);
 extern void mutex_lock_io(struct mutex *lock);
@@ -181,7 +181,7 @@ extern void mutex_lock_io(struct mutex *lock);
  * Returns 1 if the mutex has been acquired successfully, and 0 on contention.
  */
 extern int mutex_trylock(struct mutex *lock);
-extern void mutex_unlock(struct mutex *lock);
+extern void mutex_unlock(struct mutex *lock) __releases(lock);
 
 extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
 
-- 
2.24.1

[PATCH 1/3] time: Add missing annotation to lock_hrtimer_base()

[Jules Irenge]

Sparse reports a warning at lock_hrtimer_base()

|warning: context imbalance in lock_hrtimer_base() - wrong count at exit
|warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
|warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
|warning: context imbalance in __hrtimer_get_remaining()
|- unexpected unlock

To fix this , an __acquires(timer) annotation is added.
Given that lock_hrtimer_base() does actually call READ_ONCE(timer->base).
This not only fixes the warnings
but also improves on readability of the code.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/time/hrtimer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 8de90ea31280..8f555b49395a 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -160,6 +160,7 @@ static inline bool is_migration_base(struct hrtimer_clock_base *base)
 static
 struct hrtimer_clock_base *lock_hrtimer_base(const struct hrtimer *timer,
 					     unsigned long *flags)
+	__acquires(timer)
 {
 	struct hrtimer_clock_base *base;
 
-- 
2.24.1

Re: [PATCH 1/3] time: Add missing annotation to lock_hrtimer_base()

[Thomas Gleixner]

Jules,

Jules Irenge <jbi.octave@gmail.com> writes:

Please use the proper subsystem prefixes when sending patches.

git log --oneline path/to/file

gives you usally a pretty good hint.

> Sparse reports a warning at lock_hrtimer_base()
>
> |warning: context imbalance in lock_hrtimer_base() - wrong count at exit

This leading '|' is pointless

> |warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
> |warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
> |warning: context imbalance in __hrtimer_get_remaining()
> |- unexpected unlock

How are the last 3 related to:

> Sparse reports a warning at lock_hrtimer_base()

?

> To fix this , an __acquires(timer) annotation is added.

  Add the missing __acquires(timer) annotation.

Is precise and follows the recommendations of Documentation/process/...

> Given that lock_hrtimer_base() does actually call READ_ONCE(timer->base).

Given that the above sentence uses 'Given that' it should not terminate
right after explaining the 'Given'.

> This not only fixes the warnings but also improves on readability of
> the code.

I tend to disagree. In fact the annotation disturbes the reading flow
because it's on a separate line.

Can you please stop using this boilerplate which is neither helping
review nor giving someone who looks at the commit later on any useful
information?

Here is a suggestion for a change log for this:

  Sparse reports several warnings;
   warning: context imbalance in lock_hrtimer_base() - wrong count at exit
   warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
   warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
   warning: context imbalance in __hrtimer_get_remaining()- unexpected unlock

  The root cause is a missing annotation of lock_hrtimer_base() which
  causes also the 'unexpected unlock' warnings.
  
  Add the missing __acquires(timer) annotation.

Hmm?

The other 2 patches of this series have similar issues. The futex
changelog is also horribly formatted.

Thanks,

        tglx

Re: [PATCH 1/3] time: Add missing annotation to lock_hrtimer_base()

[Jules Irenge]

Thanks Thomas. I really appreciate your feedback, I take good note and I 
will send a different version with all the changes in reference to the 
email. 
 
Thanks again,
Kind regards
Jules

On Fri, 24 Jan 2020, Thomas Gleixner wrote:

> Jules,
> 
> Jules Irenge <jbi.octave@gmail.com> writes:
> 
> Please use the proper subsystem prefixes when sending patches.
> 
> git log --oneline path/to/file
> 
> gives you usally a pretty good hint.
> 
> > Sparse reports a warning at lock_hrtimer_base()
> >
> > |warning: context imbalance in lock_hrtimer_base() - wrong count at exit
> 
> This leading '|' is pointless
> 
> > |warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
> > |warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
> > |warning: context imbalance in __hrtimer_get_remaining()
> > |- unexpected unlock
> 
> How are the last 3 related to:
> 
> > Sparse reports a warning at lock_hrtimer_base()
> 
> ?
> 
> > To fix this , an __acquires(timer) annotation is added.
> 
>   Add the missing __acquires(timer) annotation.
> 
> Is precise and follows the recommendations of Documentation/process/...
> 
> > Given that lock_hrtimer_base() does actually call READ_ONCE(timer->base).
> 
> Given that the above sentence uses 'Given that' it should not terminate
> right after explaining the 'Given'.
> 
> > This not only fixes the warnings but also improves on readability of
> > the code.
> 
> I tend to disagree. In fact the annotation disturbes the reading flow
> because it's on a separate line.
> 
> Can you please stop using this boilerplate which is neither helping
> review nor giving someone who looks at the commit later on any useful
> information?
> 
> Here is a suggestion for a change log for this:
> 
>   Sparse reports several warnings;
>    warning: context imbalance in lock_hrtimer_base() - wrong count at exit
>    warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
>    warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
>    warning: context imbalance in __hrtimer_get_remaining()- unexpected unlock
> 
>   The root cause is a missing annotation of lock_hrtimer_base() which
>   causes also the 'unexpected unlock' warnings.
>   
>   Add the missing __acquires(timer) annotation.
> 
> Hmm?
> 
> The other 2 patches of this series have similar issues. The futex
> changelog is also horribly formatted.
> 
> Thanks,
> 
>         tglx
> 

Re: [PATCH 3/3] mutex: Add missing annotations

[Peter Zijlstra]

On Fri, Jan 24, 2020 at 08:12:20PM +0000, Jules Irenge wrote:
> Sparse reports false warnings and hide real warnings
> where mutex_lock() and mutex_unlock() are used within the kernel
> An example is within the kernel cgroup files
> where the below warnings are found
> |warning: context imbalance in cgroup_lock_and_drain_offline()
> | - wrong count at exit
> |warning: context imbalance in cgroup_procs_write_finish()
> |- wrong count at exit
> |warning: context imbalance in cgroup_procs_write_start()
> |- wrong count at exit.
> 
> To fix these,
> an __acquires(lock) is added to mutex_lock() declaration
> a __releases(lock) to mutex_unlock() declaration
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  include/linux/mutex.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index aca8f36dfac9..a8ab4029913e 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -162,7 +162,7 @@ do {									\
>  } while (0)
>  
>  #else
> -extern void mutex_lock(struct mutex *lock);
> +extern void mutex_lock(struct mutex *lock) __acquires(lock);
>  extern int __must_check mutex_lock_interruptible(struct mutex *lock);
>  extern int __must_check mutex_lock_killable(struct mutex *lock);
>  extern void mutex_lock_io(struct mutex *lock);
> @@ -181,7 +181,7 @@ extern void mutex_lock_io(struct mutex *lock);
>   * Returns 1 if the mutex has been acquired successfully, and 0 on contention.
>   */
>  extern int mutex_trylock(struct mutex *lock);
> -extern void mutex_unlock(struct mutex *lock);
> +extern void mutex_unlock(struct mutex *lock) __releases(lock);
>  
>  extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);

*groan*, I despise these sparse things.

The proposed patch only annotates a tiny part of the mutex interface,
and will thereby generate a flood of new (pointless) warnings. Worse,
annotating them all properly will require that __cond_lock() trainwreck.

[PATCH 0/3] Lock warning cleanups

[Jules Irenge]

This patch series adds missing annotations to functions that register
warnings of context imbalance when built with Sparse tool.
The adds fix these warnings and give insight on what the functions are
actually doing.

1. futex: a __must_hold(q->lock_ptr) is added as fixup_pi_state_owner() hold the
             lock at entry and exit.

2. futex: a __releases(&pi_state->pi_mutex.wait_lock) is added as
             wake_futex_pi() releases the lock at exit.

3. hrtimer: an __acquires() annotation is added as lock_hrtimer_base does actually call READ_ONCE().
            This add fixes the warning on other functions that call lock_hrtimer_base()

Jules Irenge (3):
  hrtimer: Add missing annotation to lock_hrtimer_base()
  futex: Add missing annotation for wake_futex_pi()
  futex: Add missing annotation for fixup_pi_state_owner()

 kernel/futex.c        | 2 ++
 kernel/time/hrtimer.c | 1 +
 2 files changed, 3 insertions(+)

-- 
2.24.1

[PATCH 1/3] hrtimer: Add missing annotation to lock_hrtimer_base()

[Jules Irenge]

Sparse reports several warnings;
warning: context imbalance in lock_hrtimer_base() - wrong count at exit
warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
warning: context imbalance in __hrtimer_get_remaining() - unexpected unlock

The root cause is a missing annotation of lock_hrtimer_base() which
causes also the "unexpected unlock" warnings.

Add the missing __acquires(timer->base) annotation

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/time/hrtimer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 3a609e7344f3..bb8340e2a3b9 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -160,6 +160,7 @@ static inline bool is_migration_base(struct hrtimer_clock_base *base)
 static
 struct hrtimer_clock_base *lock_hrtimer_base(const struct hrtimer *timer,
 					     unsigned long *flags)
+	__acquires(timer->base)
 {
 	struct hrtimer_clock_base *base;
 
-- 
2.24.1

[PATCH 2/3] futex: Add missing annotation for wake_futex_pi()

[Jules Irenge]

Sparse reports a warning at wake_futex_pi()

warning: context imbalance in wake_futex_pi() - unexpected unlock

The root cause is amissing annotation of wake_futex_pi().

Add the missing __releases(&pi_state->pi_mutex.wait_lock) annotation

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/futex.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/futex.c b/kernel/futex.c
index 0cf84c8664f2..93e7510a5b36 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1550,6 +1550,7 @@ static void mark_wake_futex(struct wake_q_head *wake_q, struct futex_q *q)
  * Caller must hold a reference on @pi_state.
  */
 static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_pi_state *pi_state)
+	__releases(&pi_state->pi_mutex.wait_lock)
 {
 	u32 uninitialized_var(curval), newval;
 	struct task_struct *new_owner;
-- 
2.24.1

[PATCH 3/3] futex: Add missing annotation for fixup_pi_state_owner()

[Jules Irenge]

Sparse reports a warning at fixup_pi_state_owner()

warning: context imbalance in fixup_pi_state_owner() - unexpected unlock

The root cause is a missing annotation of fixup_pi_state_owner().

Add the missing __must_hold(q->lock_ptr) annotation

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/futex.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/futex.c b/kernel/futex.c
index 93e7510a5b36..5263cce46c06 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2440,6 +2440,7 @@ static void unqueue_me_pi(struct futex_q *q)
 
 static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 				struct task_struct *argowner)
+	__must_hold(q->lock_ptr)
 {
 	struct futex_pi_state *pi_state = q->pi_state;
 	u32 uval, uninitialized_var(curval), newval;
-- 
2.24.1

Re: [PATCH 3/3] mutex: Add missing annotations

[Jules Irenge]

Thanks for the feedback, I did not have a thorough analyse.

I just realised it. Yes, some warnings generated after are 
pointless others look genuine. Next time I will 
have a thorough analyse and test before sending. 

I apologise for this.  

I really appreciate your feedback.

Kind regards,
Jules

On Mon, 27 Jan 2020, Peter Zijlstra wrote:

> On Fri, Jan 24, 2020 at 08:12:20PM +0000, Jules Irenge wrote:
> > Sparse reports false warnings and hide real warnings
> > where mutex_lock() and mutex_unlock() are used within the kernel
> > An example is within the kernel cgroup files
> > where the below warnings are found
> > |warning: context imbalance in cgroup_lock_and_drain_offline()
> > | - wrong count at exit
> > |warning: context imbalance in cgroup_procs_write_finish()
> > |- wrong count at exit
> > |warning: context imbalance in cgroup_procs_write_start()
> > |- wrong count at exit.
> > 
> > To fix these,
> > an __acquires(lock) is added to mutex_lock() declaration
> > a __releases(lock) to mutex_unlock() declaration
> > 
> > Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> > ---
> >  include/linux/mutex.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> > index aca8f36dfac9..a8ab4029913e 100644
> > --- a/include/linux/mutex.h
> > +++ b/include/linux/mutex.h
> > @@ -162,7 +162,7 @@ do {									\
> >  } while (0)
> >  
> >  #else
> > -extern void mutex_lock(struct mutex *lock);
> > +extern void mutex_lock(struct mutex *lock) __acquires(lock);
> >  extern int __must_check mutex_lock_interruptible(struct mutex *lock);
> >  extern int __must_check mutex_lock_killable(struct mutex *lock);
> >  extern void mutex_lock_io(struct mutex *lock);
> > @@ -181,7 +181,7 @@ extern void mutex_lock_io(struct mutex *lock);
> >   * Returns 1 if the mutex has been acquired successfully, and 0 on contention.
> >   */
> >  extern int mutex_trylock(struct mutex *lock);
> > -extern void mutex_unlock(struct mutex *lock);
> > +extern void mutex_unlock(struct mutex *lock) __releases(lock);
> >  
> >  extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
> 
> *groan*, I despise these sparse things.
> 
> The proposed patch only annotates a tiny part of the mutex interface,
> and will thereby generate a flood of new (pointless) warnings. Worse,
> annotating them all properly will require that __cond_lock() trainwreck.
> 
>