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

[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

[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

[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 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 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

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
> 

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