[PATCH 0/2] Lock warning clean up

[PATCH 0/2] Lock warning clean up

[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.
In the core kernel,
1. RCU:  a __releases() annotation is added as the functin exit trhe
critical section
2. acct: Multiple warnings were reported, adding  __releases(RCU) to the
fs_pin header file function declaration location clears these warnings
as the function releases RCU lock at exit.
Jules Irenge (2):
  fs_pin: Add missing annotation to pin_kill() declaration
  rcu/nocb: Add missing annotation for rcu_nocb_bypass_unlock()

 include/linux/fs_pin.h   | 2 +-
 kernel/rcu/tree_plugin.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.24.1

[PATCH 1/2] fs_pin: Add missing annotation to pin_kill() declaration

[Jules Irenge]

Sparse reports several warnings within the kernel acct.c file

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 a missing annotation at
declaration of pin_kill() within the header file
Given that pin_kill() does actually call
read_rcu_unlock()in its function definition

Add the missing __releases(RCU) annotation.

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 2/2] rcu/nocb: Add missing annotation for rcu_nocb_bypass_unlock()

[Jules Irenge]

Sparse reports warning at rcu_nocb_bypass_unlock()

warning: context imbalance in rcu_nocb_bypass_unlock() - unexpected unlock

The root cause is a missing annotation of rcu_nocb_bypass_unlock()
which causes the warning.

Add the missing __releases(&rdp->nocb_bypass_lock) annotation.

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

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 9d21cb07d57c..8783d19a58b2 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -1553,6 +1553,7 @@ static bool rcu_nocb_bypass_trylock(struct rcu_data *rdp)
  * Release the specified rcu_data structure's ->nocb_bypass_lock.
  */
 static void rcu_nocb_bypass_unlock(struct rcu_data *rdp)
+	__releases(&rdp->nocb_bypass_lock)
 {
 	lockdep_assert_irqs_disabled();
 	raw_spin_unlock(&rdp->nocb_bypass_lock);
-- 
2.24.1

Re: [PATCH 2/2] rcu/nocb: Add missing annotation for rcu_nocb_bypass_unlock()

[Boqun Feng]

On Thu, Jan 30, 2020 at 12:30:09AM +0000, Jules Irenge wrote:
> Sparse reports warning at rcu_nocb_bypass_unlock()
> 
> warning: context imbalance in rcu_nocb_bypass_unlock() - unexpected unlock
> 
> The root cause is a missing annotation of rcu_nocb_bypass_unlock()
> which causes the warning.
> 
> Add the missing __releases(&rdp->nocb_bypass_lock) annotation.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Acked-by: Boqun Feng <boqun.feng@gmail.com>

> ---
>  kernel/rcu/tree_plugin.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 9d21cb07d57c..8783d19a58b2 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1553,6 +1553,7 @@ static bool rcu_nocb_bypass_trylock(struct rcu_data *rdp)
>   * Release the specified rcu_data structure's ->nocb_bypass_lock.
>   */
>  static void rcu_nocb_bypass_unlock(struct rcu_data *rdp)
> +	__releases(&rdp->nocb_bypass_lock)
>  {
>  	lockdep_assert_irqs_disabled();
>  	raw_spin_unlock(&rdp->nocb_bypass_lock);
> -- 
> 2.24.1
> 

Re: [PATCH 2/2] rcu/nocb: Add missing annotation for rcu_nocb_bypass_unlock()

[Paul E. McKenney]

On Fri, Jan 31, 2020 at 08:59:52AM +0800, Boqun Feng wrote:
> On Thu, Jan 30, 2020 at 12:30:09AM +0000, Jules Irenge wrote:
> > Sparse reports warning at rcu_nocb_bypass_unlock()
> > 
> > warning: context imbalance in rcu_nocb_bypass_unlock() - unexpected unlock
> > 
> > The root cause is a missing annotation of rcu_nocb_bypass_unlock()
> > which causes the warning.
> > 
> > Add the missing __releases(&rdp->nocb_bypass_lock) annotation.
> > 
> > Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> 
> Acked-by: Boqun Feng <boqun.feng@gmail.com>

Applied, thank you both!

							Thanx, Paul

> > ---
> >  kernel/rcu/tree_plugin.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 9d21cb07d57c..8783d19a58b2 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -1553,6 +1553,7 @@ static bool rcu_nocb_bypass_trylock(struct rcu_data *rdp)
> >   * Release the specified rcu_data structure's ->nocb_bypass_lock.
> >   */
> >  static void rcu_nocb_bypass_unlock(struct rcu_data *rdp)
> > +	__releases(&rdp->nocb_bypass_lock)
> >  {
> >  	lockdep_assert_irqs_disabled();
> >  	raw_spin_unlock(&rdp->nocb_bypass_lock);
> > -- 
> > 2.24.1
> > 

[PATCH 0/2] Lock warning cleanup

[Jules Irenge]

This patchset proposes a solution to functions that regiter context
imbalance warnin, we add annotations to fix the warnings.

Jules Irenge (2):
  cxgb4: Add missing annotation for service_ofldq()
  spi: atmel: Add missing annotation for
    atmel_spi_next_xfer_dma_submit()

 drivers/net/ethernet/chelsio/cxgb4/sge.c | 1 +
 drivers/spi/spi-atmel.c                  | 1 +
 2 files changed, 2 insertions(+)

-- 
2.26.2

[PATCH 1/2] cxgb4: Add missing annotation for service_ofldq()

[Jules Irenge]

Sparse reports a warning at service_ofldq()

warning: context imbalance in service_ofldq() - unexpected unlock

The root cause is the missing annotation at service_ofldq()

Add the missing __must_hold(&q->sendq.lock) annotation

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 drivers/net/ethernet/chelsio/cxgb4/sge.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index 97cda501e7e8..5da8eb6eb9b3 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -2702,6 +2702,7 @@ static void ofldtxq_stop(struct sge_uld_txq *q, struct fw_wr_hdr *wr)
  *	is ever running at a time ...
  */
 static void service_ofldq(struct sge_uld_txq *q)
+	__must_hold(&q->sendq.lock)
 {
 	u64 *pos, *before, *end;
 	int credits;
-- 
2.26.2

[PATCH 2/2] spi: atmel: Add missing annotation for atmel_spi_next_xfer_dma_submit()

[Jules Irenge]

Sparse reports a warning at atmel_spi_next_xfer_dma_submit()

warning: context imbalance in atmel_spi_next_xfer_dma_submit()
	- unexpected unlock

The root cause is the missing annotation
	at atmel_spi_next_xfer_dma_submit()

Add the missing __must_hold(&as->lock) annotation

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

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 013458cabe3c..57ee8c3b7972 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -706,6 +706,7 @@ static void atmel_spi_next_xfer_pio(struct spi_master *master,
 static int atmel_spi_next_xfer_dma_submit(struct spi_master *master,
 				struct spi_transfer *xfer,
 				u32 *plen)
+	__must_hold(&as->lock)
 {
 	struct atmel_spi	*as = spi_master_get_devdata(master);
 	struct dma_chan		*rxchan = master->dma_rx;
-- 
2.26.2

Re: [PATCH 0/2] Lock warning cleanup

[Mark Brown]

On Wed, 29 Apr 2020 23:57:21 +0100, Jules Irenge wrote:
> This patchset proposes a solution to functions that regiter context
> imbalance warnin, we add annotations to fix the warnings.
> 
> Jules Irenge (2):
>   cxgb4: Add missing annotation for service_ofldq()
>   spi: atmel: Add missing annotation for
>     atmel_spi_next_xfer_dma_submit()
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.8

Thanks!

[1/2] cxgb4: Add missing annotation for service_ofldq()
      commit: d7f27df50eea54fd00c26c5dda7bc12d2541e5e4
[2/2] spi: atmel: Add missing annotation for atmel_spi_next_xfer_dma_submit()
      commit: e124e205124c7ab1d35ab19a45b9a70fe4f17d49

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 0/2] Lock warning cleanup

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

On Thu, Apr 30, 2020 at 03:23:17PM +0100, Mark Brown wrote:

> [1/2] cxgb4: Add missing annotation for service_ofldq()
>       commit: d7f27df50eea54fd00c26c5dda7bc12d2541e5e4

Sorry, I didn't register that this was part of a series when I
downloaded the mailbox - I'll drop that commit.  When sending small
cleanups like this to unrelated subsystems putting them in a series, it
makes it look like there's dependencies.  This is especially true if
you're not CCing the whole series to everyone, that's often a sign that
you can just separate the cleanups.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/2] audit: change unnecessary globals into statics

[Jules Irenge]

Variables sig_pid, audit_sig_uid and audit_sig_sid
are only used in the audit.c file across the kernel
Hence it appears no reason for declaring them as globals
This patch removes their global declarations from the .h file
and change them into static in the .c file.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/audit.c | 6 +++---
 kernel/audit.h | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index b2301bdc9773..afd7827cf6e8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -123,9 +123,9 @@ static u32	audit_backlog_limit = 64;
 static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
 
 /* The identity of the user shutting down the audit system. */
-kuid_t		audit_sig_uid = INVALID_UID;
-pid_t		audit_sig_pid = -1;
-u32		audit_sig_sid = 0;
+static kuid_t		audit_sig_uid = INVALID_UID;
+static pid_t		audit_sig_pid = -1;
+static u32		audit_sig_sid = 0;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]
diff --git a/kernel/audit.h b/kernel/audit.h
index ddc22878433d..3b9c0945225a 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -327,10 +327,6 @@ static inline int audit_signal_info_syscall(struct task_struct *t)
 
 extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
 
-extern pid_t audit_sig_pid;
-extern kuid_t audit_sig_uid;
-extern u32 audit_sig_sid;
-
 extern int audit_filter(int msgtype, unsigned int listtype);
 
 extern void audit_ctl_lock(void);
-- 
2.26.2

[PATCH 2/2] audit: uninitialize variable audit_sig_sid

[Jules Irenge]

Checkpatch tool reports

"ERROR: do not initialise globals/statics to 0"

To fix this, audit_sig_sid is uninitialized
As this is stored in the .bss section,
the compiler can initialize the variable automatically.

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

diff --git a/kernel/audit.c b/kernel/audit.c
index afd7827cf6e8..1c74d1d788b6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -125,7 +125,7 @@ static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
 /* The identity of the user shutting down the audit system. */
 static kuid_t		audit_sig_uid = INVALID_UID;
 static pid_t		audit_sig_pid = -1;
-static u32		audit_sig_sid = 0;
+static u32		audit_sig_sid;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]
-- 
2.26.2

[RESEND PATCH 0/2] cleanups

[Jules Irenge]

I am proposing these 2 patches, one of which is recommendation of Joe Perches,
namely the removing of unnecessary globals and change them to static.
I am currently learning the core kernel the hard way.
I will appreciate any feedback negative or positive.
Thanks

Jules Irenge (2):
  audit: change unnecessary globals into statics
  audit: uninitialize variable audit_sig_sid

 kernel/audit.c | 6 +++---
 kernel/audit.h | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

-- 
2.26.2

[RESEND PATCH 1/2] audit: change unnecessary globals into statics

[Jules Irenge]

Variables sig_pid, audit_sig_uid and audit_sig_sid
are only used in the audit.c file across the kernel
Hence it appears no reason for declaring them as globals
This patch removes their global declarations from the .h file
and change them into static in the .c file.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/audit.c | 6 +++---
 kernel/audit.h | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index b2301bdc9773..afd7827cf6e8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -123,9 +123,9 @@ static u32	audit_backlog_limit = 64;
 static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
 
 /* The identity of the user shutting down the audit system. */
-kuid_t		audit_sig_uid = INVALID_UID;
-pid_t		audit_sig_pid = -1;
-u32		audit_sig_sid = 0;
+static kuid_t		audit_sig_uid = INVALID_UID;
+static pid_t		audit_sig_pid = -1;
+static u32		audit_sig_sid = 0;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]
diff --git a/kernel/audit.h b/kernel/audit.h
index ddc22878433d..3b9c0945225a 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -327,10 +327,6 @@ static inline int audit_signal_info_syscall(struct task_struct *t)
 
 extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
 
-extern pid_t audit_sig_pid;
-extern kuid_t audit_sig_uid;
-extern u32 audit_sig_sid;
-
 extern int audit_filter(int msgtype, unsigned int listtype);
 
 extern void audit_ctl_lock(void);
-- 
2.26.2

[RESEND PATCH 2/2] audit: uninitialize variable audit_sig_sid

[Jules Irenge]

Checkpatch tool reports

"ERROR: do not initialise globals/statics to 0"

To fix this, audit_sig_sid is uninitialized
As this is stored in the .bss section,
the compiler can initialize the variable automatically.

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

diff --git a/kernel/audit.c b/kernel/audit.c
index afd7827cf6e8..1c74d1d788b6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -125,7 +125,7 @@ static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
 /* The identity of the user shutting down the audit system. */
 static kuid_t		audit_sig_uid = INVALID_UID;
 static pid_t		audit_sig_pid = -1;
-static u32		audit_sig_sid = 0;
+static u32		audit_sig_sid;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]
-- 
2.26.2

Re: [RESEND PATCH 1/2] audit: change unnecessary globals into statics

[Paul Moore]

On Mon, Aug 3, 2020 at 8:35 AM Jules Irenge <jbi.octave@gmail.com> wrote:
>
> Variables sig_pid, audit_sig_uid and audit_sig_sid
> are only used in the audit.c file across the kernel
> Hence it appears no reason for declaring them as globals
> This patch removes their global declarations from the .h file
> and change them into static in the .c file.
>
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  kernel/audit.c | 6 +++---
>  kernel/audit.h | 4 ----
>  2 files changed, 3 insertions(+), 7 deletions(-)

Thanks Jules, this looks reasonable although I'm not going to merge
them into audit/next until after the merge window closes.  I'll send
another reply once this has been merged.

-- 
paul moore
www.paul-moore.com

Re: [RESEND PATCH 2/2] audit: uninitialize variable audit_sig_sid

[Paul Moore]

On Mon, Aug 3, 2020 at 8:35 AM Jules Irenge <jbi.octave@gmail.com> wrote:
>
> Checkpatch tool reports
>
> "ERROR: do not initialise globals/statics to 0"
>
> To fix this, audit_sig_sid is uninitialized
> As this is stored in the .bss section,
> the compiler can initialize the variable automatically.
>
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  kernel/audit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Similar to patch 1/2, this will need to wait until after the merge
window closes.

-- 
paul moore
www.paul-moore.com

Re: [RESEND PATCH 1/2] audit: change unnecessary globals into statics

[Paul Moore]

On Thu, Aug 6, 2020 at 2:33 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Aug 3, 2020 at 8:35 AM Jules Irenge <jbi.octave@gmail.com> wrote:
> >
> > Variables sig_pid, audit_sig_uid and audit_sig_sid
> > are only used in the audit.c file across the kernel
> > Hence it appears no reason for declaring them as globals
> > This patch removes their global declarations from the .h file
> > and change them into static in the .c file.
> >
> > Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> > ---
> >  kernel/audit.c | 6 +++---
> >  kernel/audit.h | 4 ----
> >  2 files changed, 3 insertions(+), 7 deletions(-)
>
> Thanks Jules, this looks reasonable although I'm not going to merge
> them into audit/next until after the merge window closes.  I'll send
> another reply once this has been merged.

... and I just merged this into audit/next, thanks Jules.

-- 
paul moore
www.paul-moore.com

Re: [RESEND PATCH 2/2] audit: uninitialize variable audit_sig_sid

[Paul Moore]

On Thu, Aug 6, 2020 at 2:35 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Aug 3, 2020 at 8:35 AM Jules Irenge <jbi.octave@gmail.com> wrote:
> >
> > Checkpatch tool reports
> >
> > "ERROR: do not initialise globals/statics to 0"
> >
> > To fix this, audit_sig_sid is uninitialized
> > As this is stored in the .bss section,
> > the compiler can initialize the variable automatically.
> >
> > Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> > ---
> >  kernel/audit.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Similar to patch 1/2, this will need to wait until after the merge
> window closes.

... also merged this patch into audit/next.  Thanks again.

-- 
paul moore
www.paul-moore.com