[PATCH 00/11] Lock warning cleanup

[PATCH 00/11] Lock warning cleanup

[Jules Irenge]

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

1. Within the futex subsystem, a __releases(&pi_state->.pi_mutex.wait_lock) is added because wake_futex_pi() only releases the lock at exit,
must_hold(q->lock_ptr) have been added to fixup_pi_state_owner() because the lock is held at entry and exit;
a __releases(&hb->lock) added to futex_wait_queue_me() as it only releases the lock.

2. Within fs_pin, a __releases(RCU) is added because the function exit RCU critical section at exit.

3. In kasan, an __acquires(&report_lock) has been added to start_report() and   __releases(&report_lock) to end_report() 

4. Within ring_buffer subsystem, a __releases(RCU) has been added perf_output_end() 

5. schedule subsystem recorded an addition of the __releases(rq->lock) annotation and a __must_hold(this_rq->lock)

6. At hrtimer subsystem, __acquires(timer) is added  to lock_hrtimer_base() as the function acquire the lock but never releases it.
Jules Irenge (11):
  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()
  perf/ring_buffer: Add missing annotation to perf_output_end()
  sched/fair: Add missing annotation for nohz_newidle_balance()
  sched/deadline: Add missing annotation for dl_task_offline_migration()
  fs_pin: Add missing annotation for pin_kill() declaration
  fs_pin: Add missing annotation for pin_kill() definition
  kasan: add missing annotation for start_report()
  kasan: add missing annotation for end_report()
  futex: Add missing annotation for futex_wait_queue_me()

 fs/fs_pin.c                 | 2 +-
 include/linux/fs_pin.h      | 2 +-
 kernel/events/ring_buffer.c | 2 +-
 kernel/futex.c              | 3 +++
 kernel/sched/deadline.c     | 1 +
 kernel/sched/fair.c         | 2 +-
 kernel/time/hrtimer.c       | 1 +
 mm/kasan/report.c           | 4 ++--
 8 files changed, 11 insertions(+), 6 deletions(-)

-- 
2.24.1

[PATCH 01/11] 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 02/11] 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 03/11] 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

[PATCH 04/11] perf/ring_buffer: Add missing annotation to perf_output_end()

[Jules Irenge]

Sparse reports a warning at perf_output_end()

warning: context imbalance in perf_output_end() - unexpected unlock

The root cause is a missing annotation for perf_output_end()

Add the missing __releases(RCU) annotation

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

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 7ffd5c763f93..c09edc49a4fc 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -294,7 +294,7 @@ unsigned int perf_output_skip(struct perf_output_handle *handle,
 	return __output_skip(handle, NULL, len);
 }
 
-void perf_output_end(struct perf_output_handle *handle)
+void perf_output_end(struct perf_output_handle *handle) __releases(RCU)
 {
 	perf_output_put_handle(handle);
 	rcu_read_unlock();
-- 
2.24.1

[PATCH 05/11] sched/fair: Add missing annotation for nohz_newidle_balance()

[Jules Irenge]

Sparse reports a warning at nohz_newidle_balance()
warning: context imbalance in nohz_newidle_balance() - wrong count at exit
The root cause is a missing annotation

Add the missing __must_hold(this_rq->lock)

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

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index fe4e0d775375..81f8a440469a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10050,7 +10050,7 @@ static bool nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
 	return true;
 }
 
-static void nohz_newidle_balance(struct rq *this_rq)
+static void nohz_newidle_balance(struct rq *this_rq) __must_hold(&this_rq->lock)
 {
 	int this_cpu = this_rq->cpu;
 
-- 
2.24.1

[PATCH 06/11] sched/deadline: Add missing annotation for dl_task_offline_migration()

[Jules Irenge]

Sparse reports warning at dl_task_offline_migration()

warning: context imbalance in dl_task_offline_migration()
	 - unexpected unlock

The root cause is the missing annotation for dl_task_offline_migration()

Add the missing __releases(rq->lock) annotation.

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

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 43323f875cb9..68ea3a4933db 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -527,6 +527,7 @@ static inline void deadline_queue_pull_task(struct rq *rq)
 static struct rq *find_lock_later_rq(struct task_struct *task, struct rq *rq);
 
 static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p)
+	__releases(rq->lock)
 {
 	struct rq *later_rq = NULL;
 	struct dl_bw *dl_b;
-- 
2.24.1

[PATCH 07/11] fs_pin: Add missing annotation for pin_kill() declaration

[Jules Irenge]

Sparse reports warnings within the kernel acct.c file
at acct_exit_ns(), __se_sys_acct(), acct_on()

warning: context imbalance in acct_on()
	- different lock contexts for basic block
warning: context imbalance in __se_sys_acct()
	- different lock contexts for basic block
warning: context imbalance in acct_exit_ns()
	- wrong count at exit

The root cause is the missing annotation at pin_kill()

In fact acct_exit_ns(), __se_sys_sys_acct() and acct_on()
 do actually call rcu_read_lock()
then call pin_kill() which is defined elsewhere.
A close look at pin_kill()
- called 3 times in the core kernel and 2 times elsewhere-
shows that pin_kill() does actually call rcu_read_unlock().
Adding the annotation at declaration and definition of pin_kill()
not only fixes the warnings
but also improves on the readability of the code

Add the missing annotation __release(RCU)

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

diff --git a/include/linux/fs_pin.h b/include/linux/fs_pin.h
index bdd09fd2520c..d2fcf1a5112f 100644
--- a/include/linux/fs_pin.h
+++ b/include/linux/fs_pin.h
@@ -21,4 +21,4 @@ static inline void init_fs_pin(struct fs_pin *p, void (*kill)(struct fs_pin *))
 
 void pin_remove(struct fs_pin *);
 void pin_insert(struct fs_pin *, struct vfsmount *);
-void pin_kill(struct fs_pin *);
+void pin_kill(struct fs_pin *) __releases(RCU);
-- 
2.24.1

[PATCH 08/11] fs_pin: Add missing annotation for pin_kill() definition

[Jules Irenge]

Sparse reports a warning at pin_kill()
warning: context imbalance in pin_kil() - unexpected unlock

The root cause is a missing annotation for pin_kill()

Add the missing annotation __releases(RCU)

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

diff --git a/fs/fs_pin.c b/fs/fs_pin.c
index 47ef3c71ce90..972168453fba 100644
--- a/fs/fs_pin.c
+++ b/fs/fs_pin.c
@@ -27,7 +27,7 @@ void pin_insert(struct fs_pin *pin, struct vfsmount *m)
 	spin_unlock(&pin_lock);
 }
 
-void pin_kill(struct fs_pin *p)
+void pin_kill(struct fs_pin *p) __releases(RCU)
 {
 	wait_queue_entry_t wait;
 
-- 
2.24.1

[PATCH 09/11] kasan: add missing annotation for start_report()

[Jules Irenge]

Sparse reports a warning at start_report()

warning: context imbalance in start_report() - wrong count at exit

The root cause is a missing annotation at start_report()

Add the missing annotation __acquires(&report_lock)

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 mm/kasan/report.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 5ef9f24f566b..5451624c4e09 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -77,7 +77,7 @@ static void print_error_description(struct kasan_access_info *info)
 
 static DEFINE_SPINLOCK(report_lock);
 
-static void start_report(unsigned long *flags)
+static void start_report(unsigned long *flags) __acquires(&report_lock)
 {
 	/*
 	 * Make sure we don't end up in loop.
-- 
2.24.1

[PATCH 10/11] kasan: add missing annotation for end_report()

[Jules Irenge]

Sparse reports a warning at end_report()

warning: context imbalance in end_report() - unexpected lock

The root cause is a missing annotation at end_report()

Add the missing annotation __releases(&report_lock)

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 mm/kasan/report.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 5451624c4e09..8adaa4eaee31 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -87,7 +87,7 @@ static void start_report(unsigned long *flags) __acquires(&report_lock)
 	pr_err("==================================================================\n");
 }
 
-static void end_report(unsigned long *flags)
+static void end_report(unsigned long *flags)  __releases(&report_lock)
 {
 	pr_err("==================================================================\n");
 	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
-- 
2.24.1

[PATCH 11/11] futex: Add missing annotation for futex_wait_queue_me()

[Jules Irenge]

Sparse reports a warning at futex_wait_queue_me()

warning: context imbalance in futex_wait_queue_me() - unexpected unlock

The root cause is a missing annotation at futex_wait_queue_me()

Add the missing annotation  __releases(&hb->lock)

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 5263cce46c06..16c6c40dbd68 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2679,6 +2679,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
  */
 static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
 				struct hrtimer_sleeper *timeout)
+	__releases(&hb->lock)
 {
 	/*
 	 * The task state is guaranteed to be set before another task can
-- 
2.24.1

Re: [PATCH 00/11] Lock warning cleanup

[Boqun Feng]

Hi Jules,

On Sun, Feb 09, 2020 at 10:24:42PM +0000, Jules Irenge wrote:
> This patch series adds missing annotations to functions that register warnings of context imbalance when built with Sparse tool.
> The adds fix the warnings and give insight on what the functions are actually doing.
> 
> 1. Within the futex subsystem, a __releases(&pi_state->.pi_mutex.wait_lock) is added because wake_futex_pi() only releases the lock at exit,
> must_hold(q->lock_ptr) have been added to fixup_pi_state_owner() because the lock is held at entry and exit;
> a __releases(&hb->lock) added to futex_wait_queue_me() as it only releases the lock.
> 
> 2. Within fs_pin, a __releases(RCU) is added because the function exit RCU critical section at exit.
> 
> 3. In kasan, an __acquires(&report_lock) has been added to start_report() and   __releases(&report_lock) to end_report() 
> 
> 4. Within ring_buffer subsystem, a __releases(RCU) has been added perf_output_end() 
> 
> 5. schedule subsystem recorded an addition of the __releases(rq->lock) annotation and a __must_hold(this_rq->lock)
> 
> 6. At hrtimer subsystem, __acquires(timer) is added  to lock_hrtimer_base() as the function acquire the lock but never releases it.
> Jules Irenge (11):
>   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()

Given that those three patches have been sent and reviewed, please do
increase the version number (this time, for example, using v2) when
sending the updated ones. Also please add a few sentences after the
commit log describing what you have changed between versions.

Here is an example:

	https://lore.kernel.org/lkml/20200124231834.63628-4-pmalani@chromium.org/

Regards,
Boqun

>   perf/ring_buffer: Add missing annotation to perf_output_end()
>   sched/fair: Add missing annotation for nohz_newidle_balance()
>   sched/deadline: Add missing annotation for dl_task_offline_migration()
>   fs_pin: Add missing annotation for pin_kill() declaration
>   fs_pin: Add missing annotation for pin_kill() definition
>   kasan: add missing annotation for start_report()
>   kasan: add missing annotation for end_report()
>   futex: Add missing annotation for futex_wait_queue_me()
> 
>  fs/fs_pin.c                 | 2 +-
>  include/linux/fs_pin.h      | 2 +-
>  kernel/events/ring_buffer.c | 2 +-
>  kernel/futex.c              | 3 +++
>  kernel/sched/deadline.c     | 1 +
>  kernel/sched/fair.c         | 2 +-
>  kernel/time/hrtimer.c       | 1 +
>  mm/kasan/report.c           | 4 ++--
>  8 files changed, 11 insertions(+), 6 deletions(-)
> 
> -- 
> 2.24.1
> 

Re: [PATCH 10/11] kasan: add missing annotation for end_report()

[Dmitry Vyukov]

On Sun, Feb 9, 2020 at 11:49 PM Jules Irenge <jbi.octave@gmail.com> wrote:
>
> Sparse reports a warning at end_report()
>
> warning: context imbalance in end_report() - unexpected lock
>
> The root cause is a missing annotation at end_report()
>
> Add the missing annotation __releases(&report_lock)
>
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Acked-by: Dmitry Vyukov <dvyukov@google.com>

> ---
>  mm/kasan/report.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 5451624c4e09..8adaa4eaee31 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -87,7 +87,7 @@ static void start_report(unsigned long *flags) __acquires(&report_lock)
>         pr_err("==================================================================\n");
>  }
>
> -static void end_report(unsigned long *flags)
> +static void end_report(unsigned long *flags)  __releases(&report_lock)
>  {
>         pr_err("==================================================================\n");
>         add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> --
> 2.24.1
>

Re: [PATCH 09/11] kasan: add missing annotation for start_report()

[Dmitry Vyukov]

On Sun, Feb 9, 2020 at 11:48 PM Jules Irenge <jbi.octave@gmail.com> wrote:
>
> Sparse reports a warning at start_report()
>
> warning: context imbalance in start_report() - wrong count at exit
>
> The root cause is a missing annotation at start_report()
>
> Add the missing annotation __acquires(&report_lock)
>
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Acked-by: Dmitry Vyukov <dvyukov@google.com>

> ---
>  mm/kasan/report.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 5ef9f24f566b..5451624c4e09 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -77,7 +77,7 @@ static void print_error_description(struct kasan_access_info *info)
>
>  static DEFINE_SPINLOCK(report_lock);
>
> -static void start_report(unsigned long *flags)
> +static void start_report(unsigned long *flags) __acquires(&report_lock)
>  {
>         /*
>          * Make sure we don't end up in loop.
> --
> 2.24.1
>

Re: [PATCH 06/11] sched/deadline: Add missing annotation for dl_task_offline_migration()

[Juri Lelli]

Hi,

On 09/02/20 22:39, Jules Irenge wrote:
> Sparse reports warning at dl_task_offline_migration()
> 
> warning: context imbalance in dl_task_offline_migration()
> 	 - unexpected unlock
> 
> The root cause is the missing annotation for dl_task_offline_migration()
> 
> Add the missing __releases(rq->lock) annotation.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Acked-by: Juri Lelli <juri.lelli@redhat.com>

Best,

Juri

Re: [PATCH 00/11] Lock warning cleanup

[Jules Irenge]

On Mon, 10 Feb 2020, Boqun Feng wrote:

> Hi Jules,
> 
> On Sun, Feb 09, 2020 at 10:24:42PM +0000, Jules Irenge wrote:
> > This patch series adds missing annotations to functions that register warnings of context imbalance when built with Sparse tool.
> > The adds fix the warnings and give insight on what the functions are actually doing.
> > 
> > 1. Within the futex subsystem, a __releases(&pi_state->.pi_mutex.wait_lock) is added because wake_futex_pi() only releases the lock at exit,
> > must_hold(q->lock_ptr) have been added to fixup_pi_state_owner() because the lock is held at entry and exit;
> > a __releases(&hb->lock) added to futex_wait_queue_me() as it only releases the lock.
> > 
> > 2. Within fs_pin, a __releases(RCU) is added because the function exit RCU critical section at exit.
> > 
> > 3. In kasan, an __acquires(&report_lock) has been added to start_report() and   __releases(&report_lock) to end_report() 
> > 
> > 4. Within ring_buffer subsystem, a __releases(RCU) has been added perf_output_end() 
> > 
> > 5. schedule subsystem recorded an addition of the __releases(rq->lock) annotation and a __must_hold(this_rq->lock)
> > 
> > 6. At hrtimer subsystem, __acquires(timer) is added  to lock_hrtimer_base() as the function acquire the lock but never releases it.
> > Jules Irenge (11):
> >   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()
> 
> Given that those three patches have been sent and reviewed, please do
> increase the version number (this time, for example, using v2) when
> sending the updated ones. Also please add a few sentences after the
> commit log describing what you have changed between versions.
> 
> Here is an example:
> 
> 	https://lore.kernel.org/lkml/20200124231834.63628-4-pmalani@chromium.org/
> 
> Regards,
> Boqun
> 
> >   perf/ring_buffer: Add missing annotation to perf_output_end()
> >   sched/fair: Add missing annotation for nohz_newidle_balance()
> >   sched/deadline: Add missing annotation for dl_task_offline_migration()
> >   fs_pin: Add missing annotation for pin_kill() declaration
> >   fs_pin: Add missing annotation for pin_kill() definition
> >   kasan: add missing annotation for start_report()
> >   kasan: add missing annotation for end_report()
> >   futex: Add missing annotation for futex_wait_queue_me()
> > 
> >  fs/fs_pin.c                 | 2 +-
> >  include/linux/fs_pin.h      | 2 +-
> >  kernel/events/ring_buffer.c | 2 +-
> >  kernel/futex.c              | 3 +++
> >  kernel/sched/deadline.c     | 1 +
> >  kernel/sched/fair.c         | 2 +-
> >  kernel/time/hrtimer.c       | 1 +
> >  mm/kasan/report.c           | 4 ++--
> >  8 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.24.1
> > 
> 

Thanks for the feedback, I take good notes. I am working on the 
second version.

Kind regards,
Jules