[PATCH 0/7] Lock warnings cleanup

[PATCH 0/7] Lock warnings cleanup

[Jules Irenge]

This patch series adds missing annotations to various functions,
that register warnings of context imbalance when built with Sparse tool.
The adds fix the warnings, improve on readability odf the code
and give better insight or directive on what the functions are actually doing.

Jules Irenge (7):
  fs: Add missing annotation for iput_final()
  fsnotify: Add missing annotation for fsnotify_finish_user_wait()
  dax: Add missing annotation for wait_entry_unlocked()
  sysctl: Add missing annotation for start_unregistering()
  btrfs: Add missing annotation for btrfs_lock_cluster()
  btrfs: Add missing annotation for btrfs_tree_lock()
  tty: serial_core: Add missing annotation for _unlock_and_check_sysrq()

 drivers/tty/serial/serial_core.c | 1 +
 fs/btrfs/extent-tree.c           | 1 +
 fs/btrfs/locking.c               | 1 +
 fs/dax.c                         | 1 +
 fs/inode.c                       | 1 +
 fs/notify/mark.c                 | 1 +
 fs/proc/proc_sysctl.c            | 1 +
 7 files changed, 7 insertions(+)

-- 
2.24.1

[PATCH 1/7] fs: Add missing annotation for iput_final()

[Jules Irenge]

Sparse reports a warning at iput_final()

warning: context imbalance in iput_final() - unexpected unlock

The root cause is the missing annotation at input_final()
Add the missing __releases(&inode->i_lock) annotation

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

diff --git a/fs/inode.c b/fs/inode.c
index 3b06c5c59883..6902e39a4298 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1536,6 +1536,7 @@ EXPORT_SYMBOL(generic_delete_inode);
  * shutting down.
  */
 static void iput_final(struct inode *inode)
+	__releases(&inode->i_lock)
 {
 	struct super_block *sb = inode->i_sb;
 	const struct super_operations *op = inode->i_sb->s_op;
-- 
2.24.1

[PATCH 2/7] fsnotify: Add missing annotation for fsnotify_finish_user_wait()

[Jules Irenge]

Sparse reports a warning at fsnotify_finish_user_wait()

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

The root cause is the missing annotation at fsnotify_finish_user_wait()
Add the missing __acquires(&fsnotify_mark_srcu) annotation.

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

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 1d96216dffd1..44fea637bb02 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -350,6 +350,7 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
 }
 
 void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
+	__acquires(&fsnotify_mark_srcu)
 {
 	int type;
 
-- 
2.24.1

Re: [PATCH 2/7] fsnotify: Add missing annotation for fsnotify_finish_user_wait()

[Jan Kara]

On Tue 31-03-20 21:46:38, Jules Irenge wrote:
> Sparse reports a warning at fsnotify_finish_user_wait()
> 
> warning: context imbalance in fsnotify_finish_user_wait()
> 	- wrong count at exit
> 
> The root cause is the missing annotation at fsnotify_finish_user_wait()
> Add the missing __acquires(&fsnotify_mark_srcu) annotation.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

OK, but then fsnotify_prepare_user_wait() needs __releases annotation as
well if we're going to be serious about sparse warnings in this code?

								Honza

> ---
>  fs/notify/mark.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 1d96216dffd1..44fea637bb02 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -350,6 +350,7 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>  }
>  
>  void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
> +	__acquires(&fsnotify_mark_srcu)
>  {
>  	int type;
>  
> -- 
> 2.24.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/7] fsnotify: Add missing annotation for fsnotify_finish_user_wait()

[Jules Irenge]

On Wed, 1 Apr 2020, Jan Kara wrote:

> On Tue 31-03-20 21:46:38, Jules Irenge wrote:
>> Sparse reports a warning at fsnotify_finish_user_wait()
>>
>> warning: context imbalance in fsnotify_finish_user_wait()
>> 	- wrong count at exit
>>
>> The root cause is the missing annotation at fsnotify_finish_user_wait()
>> Add the missing __acquires(&fsnotify_mark_srcu) annotation.
>>
>> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
>
> OK, but then fsnotify_prepare_user_wait() needs __releases annotation as
> well if we're going to be serious about sparse warnings in this code?
>
> 								Honza
>
>> ---
>>  fs/notify/mark.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
>> index 1d96216dffd1..44fea637bb02 100644
>> --- a/fs/notify/mark.c
>> +++ b/fs/notify/mark.c
>> @@ -350,6 +350,7 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>>  }
>>
>>  void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
>> +	__acquires(&fsnotify_mark_srcu)
>>  {
>>  	int type;
>>
>> --
>> 2.24.1
>>
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>

Thanks for the reply. I think adding an annotation at 
fsnotify_prepare_user_wait() will not theoretically remove the warning. 
That's the only reason why I skipped it .
Best regards,
Jules

Re: [PATCH 2/7] fsnotify: Add missing annotation for fsnotify_finish_user_wait()

[Jan Kara]

On Fri 03-04-20 17:15:44, Jules Irenge wrote:
> 
> 
> On Wed, 1 Apr 2020, Jan Kara wrote:
> 
> > On Tue 31-03-20 21:46:38, Jules Irenge wrote:
> > > Sparse reports a warning at fsnotify_finish_user_wait()
> > > 
> > > warning: context imbalance in fsnotify_finish_user_wait()
> > > 	- wrong count at exit
> > > 
> > > The root cause is the missing annotation at fsnotify_finish_user_wait()
> > > Add the missing __acquires(&fsnotify_mark_srcu) annotation.
> > > 
> > > Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> > 
> > OK, but then fsnotify_prepare_user_wait() needs __releases annotation as
> > well if we're going to be serious about sparse warnings in this code?
> > 
> > 								Honza
> > 
> > > ---
> > >  fs/notify/mark.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> > > index 1d96216dffd1..44fea637bb02 100644
> > > --- a/fs/notify/mark.c
> > > +++ b/fs/notify/mark.c
> > > @@ -350,6 +350,7 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
> > >  }
> > > 
> > >  void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
> > > +	__acquires(&fsnotify_mark_srcu)
> > >  {
> > >  	int type;
> > > 
> > > --
> > > 2.24.1
> > > 
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> > 
> 
> Thanks for the reply. I think adding an annotation at
> fsnotify_prepare_user_wait() will not theoretically remove the warning.
> That's the only reason why I skipped it .

Well, I think the goal isn't really to remove warnings but to make
annotations correct... So even if sparse was not clever enough to spot that
missing annotation, you should add it if you've decided to fix sparse
annotations for fsnotify code.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/7] dax: Add missing annotation for wait_entry_unlocked()

[Jules Irenge]

Sparse reports a warning at wait_entry_unlocked()

warning: context imbalance in wait_entry_unlocked()
	- unexpected unlock

The root cause is the missing annotation at wait_entry_unlocked()
Add the missing __releases(xa) annotation.

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

diff --git a/fs/dax.c b/fs/dax.c
index 1f1f0201cad1..adcd2a57fbad 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -244,6 +244,7 @@ static void *get_unlocked_entry(struct xa_state *xas, unsigned int order)
  * After we call xas_unlock_irq(), we cannot touch xas->xa.
  */
 static void wait_entry_unlocked(struct xa_state *xas, void *entry)
+	__releases(xa)
 {
 	struct wait_exceptional_entry_queue ewait;
 	wait_queue_head_t *wq;
-- 
2.24.1

Re: [PATCH 3/7] dax: Add missing annotation for wait_entry_unlocked()

[Jan Kara]

On Tue 31-03-20 21:46:39, Jules Irenge wrote:
> Sparse reports a warning at wait_entry_unlocked()
> 
> warning: context imbalance in wait_entry_unlocked()
> 	- unexpected unlock
> 
> The root cause is the missing annotation at wait_entry_unlocked()
> Add the missing __releases(xa) annotation.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  fs/dax.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 1f1f0201cad1..adcd2a57fbad 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -244,6 +244,7 @@ static void *get_unlocked_entry(struct xa_state *xas, unsigned int order)
>   * After we call xas_unlock_irq(), we cannot touch xas->xa.
>   */
>  static void wait_entry_unlocked(struct xa_state *xas, void *entry)
> +	__releases(xa)

Thanks for the patch but is this a proper sparse annotation? I'd rather
expect something like __releases(xas->xa->xa_lock) here...

								Honza

>  {
>  	struct wait_exceptional_entry_queue ewait;
>  	wait_queue_head_t *wq;
> -- 
> 2.24.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/7] dax: Add missing annotation for wait_entry_unlocked()

[Jules Irenge]

On Wed, 1 Apr 2020, Jan Kara wrote:

> On Tue 31-03-20 21:46:39, Jules Irenge wrote:
>> Sparse reports a warning at wait_entry_unlocked()
>>
>> warning: context imbalance in wait_entry_unlocked()
>> 	- unexpected unlock
>>
>> The root cause is the missing annotation at wait_entry_unlocked()
>> Add the missing __releases(xa) annotation.
>>
>> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
>> ---
>>  fs/dax.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/dax.c b/fs/dax.c
>> index 1f1f0201cad1..adcd2a57fbad 100644
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -244,6 +244,7 @@ static void *get_unlocked_entry(struct xa_state *xas, unsigned int order)
>>   * After we call xas_unlock_irq(), we cannot touch xas->xa.
>>   */
>>  static void wait_entry_unlocked(struct xa_state *xas, void *entry)
>> +	__releases(xa)
>
> Thanks for the patch but is this a proper sparse annotation? I'd rather
> expect something like __releases(xas->xa->xa_lock) here...
>
> 								Honza
>
>>  {
>>  	struct wait_exceptional_entry_queue ewait;
>>  	wait_queue_head_t *wq;
>> --
>> 2.24.1
>>
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>
Thanks for the kind reply. I learned and changed. If there is a further 
issue, please do not hesitate to contact me.
Thanks,
Jules

[PATCH 4/7] sysctl: Add missing annotation for start_unregistering()

[Jules Irenge]

Sparse reports a warning at start_unregistering()

warning: context imbalance in start_unregistering()
	- unexpected unlock

The root cause is the missing annotation at start_unregistering()
Add the missing __must_hold(&sysctl_lock) annotation.

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

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index c75bb4632ed1..d1b5e2b35564 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -307,6 +307,7 @@ static void proc_sys_prune_dcache(struct ctl_table_header *head)
 
 /* called under sysctl_lock, will reacquire if has to wait */
 static void start_unregistering(struct ctl_table_header *p)
+	__must_hold(&sysctl_lock)
 {
 	/*
 	 * if p->used is 0, nobody will ever touch that entry again;
-- 
2.24.1

Re: [PATCH 4/7] sysctl: Add missing annotation for start_unregistering()

[Luis Chamberlain]

On Tue, Mar 31, 2020 at 09:46:40PM +0100, Jules Irenge wrote:
> Sparse reports a warning at start_unregistering()
> 
> warning: context imbalance in start_unregistering()
> 	- unexpected unlock
> 
> The root cause is the missing annotation at start_unregistering()
> Add the missing __must_hold(&sysctl_lock) annotation.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis
> ---
>  fs/proc/proc_sysctl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index c75bb4632ed1..d1b5e2b35564 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -307,6 +307,7 @@ static void proc_sys_prune_dcache(struct ctl_table_header *head)
>  
>  /* called under sysctl_lock, will reacquire if has to wait */
>  static void start_unregistering(struct ctl_table_header *p)
> +	__must_hold(&sysctl_lock)
>  {
>  	/*
>  	 * if p->used is 0, nobody will ever touch that entry again;
> -- 
> 2.24.1
> 

[PATCH 5/7] btrfs: Add missing annotation for btrfs_lock_cluster()

[Jules Irenge]

Sparse reports a warning at btrfs_lock_cluster()

warning: context imbalance in btrfs_lock_cluster()
	- wrong count

The root cause is the missing annotation at btrfs_lock_cluster()
Add the missing __acquires(&cluster->refill_lock) annotation.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 0163fdd59f8f..9057a5ca6678 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3395,6 +3395,7 @@ static struct btrfs_block_group *btrfs_lock_cluster(
 		   struct btrfs_block_group *block_group,
 		   struct btrfs_free_cluster *cluster,
 		   int delalloc)
+	__acquires(&cluster->refill_lock)
 {
 	struct btrfs_block_group *used_bg = NULL;
 
-- 
2.24.1

Re: [PATCH 5/7] btrfs: Add missing annotation for btrfs_lock_cluster()

[David Sterba]

On Tue, Mar 31, 2020 at 09:46:41PM +0100, Jules Irenge wrote:
> Sparse reports a warning at btrfs_lock_cluster()
> 
> warning: context imbalance in btrfs_lock_cluster()
> 	- wrong count
> 
> The root cause is the missing annotation at btrfs_lock_cluster()
> Add the missing __acquires(&cluster->refill_lock) annotation.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Thanks, I'll add it to devel queue. 

[PATCH 6/7] btrfs: Add missing annotation for btrfs_tree_lock()

[Jules Irenge]

Sparse reports a warning at btrfs_tree_lock()

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

The root cause is the missing annotation at btrfs_tree_lock()
Add the missing __acquires(&eb->lock) annotation

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

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 571c4826c428..850fae45a8a4 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -410,6 +410,7 @@ void btrfs_tree_read_unlock_blocking(struct extent_buffer *eb)
  * The rwlock is held for write upon exit.
  */
 void btrfs_tree_lock(struct extent_buffer *eb)
+	__acquires(&eb->lock)
 {
 	u64 start_ns = 0;
 
-- 
2.24.1

[PATCH 7/7] tty: serial_core: Add missing annotation for _unlock_and_check_sysrq()

[Jules Irenge]

Sparse reports a warning at _unlock_and_check_sysrq()

warning: context imbalance in uart_unlock_and_check_sysrq()
	- unexpected unlock

The root cause is the missing annotation at _unlock_and_check_sysrq()
Add the missing __releases(&port->lock) annotation.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 76e506ee335c..32f93f03efce 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3121,6 +3121,7 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
 EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
 
 void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+	__releases(&port->lock)
 {
 	int sysrq_ch;
 
-- 
2.24.1

Re: [PATCH 7/7] tty: serial_core: Add missing annotation for _unlock_and_check_sysrq()

[Greg Kroah-Hartman]

On Tue, Mar 31, 2020 at 09:46:43PM +0100, Jules Irenge wrote:
> Sparse reports a warning at _unlock_and_check_sysrq()
> 
> warning: context imbalance in uart_unlock_and_check_sysrq()
> 	- unexpected unlock
> 
> The root cause is the missing annotation at _unlock_and_check_sysrq()
> Add the missing __releases(&port->lock) annotation.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  drivers/tty/serial/serial_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 76e506ee335c..32f93f03efce 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3121,6 +3121,7 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
>  EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
>  
>  void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
> +	__releases(&port->lock)
>  {
>  	int sysrq_ch;
>  
> -- 
> 2.24.1
> 

This is already in the tree, are you sure you are not working from an
older one?

greg k-h