"unexpected unlock" when unlocking, conditional, lock in loop

"unexpected unlock" when unlocking, conditional, lock in loop

[ecashin]

Hi.  I have a function that enters with a lock held and does
an unlock inside a loop.

Sparse 0.4.4 is fine with this function until I introduce a conditional
between the unlock and the next lock.  In the minimal example below,
changing the "#if 1" to "#if 0" makes sparse generate the warning
below:

cd ~/git/linux && PATH=/opt/bin:$PATH make drivers/block/aoe/aoe.ko C=1
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `relocs'.
  CHK     include/linux/version.h
  CHK     include/generated/utsrelease.h
  CALL    scripts/checksyscalls.sh
  CHECK   drivers/block/aoe/demo.c
/build/ecashin/git/linux/arch/x86/include/asm/spinlock.h:81:9: warning: context imbalance in 'demofn' - unexpected unlock
  CC [M]  drivers/block/aoe/demo.o
  LD [M]  drivers/block/aoe/aoe.o
  MODPOST 1 modules
  CC      drivers/block/aoe/aoe.mod.o
  LD [M]  drivers/block/aoe/aoe.ko

I'm using 3.6.0-rc7 kernel sources.

Granted, I'm unusually tired today, but I can't think of a way that
conditionally printing a warning has changed the locking, so I
could use some help in determining whether this is a sparse bug
that might be fixed, one that I have to work around, or some
confusion of mine.

/* demo.c */
#include <linux/netdevice.h>

static spinlock_t lk;
static struct sk_buff_head q;
int demofn(void);

/* enters and returns with lk held */
int demofn(void)
{
	struct sk_buff *skb;

	while ((skb = skb_dequeue(&q))) {
		spin_unlock_irq(&lk);
#if 1
		dev_queue_xmit(skb);
#else
		if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
			pr_warn("informative warning\n");
#endif
		spin_lock_irq(&lk);
	}
	return 0;
}

-- 
  Ed Cashin
  ecashin@coraid.com	http://www.coraid.com/
  ecashin@noserose.net	http://noserose.net/e/

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Josh Triplett]

On Sat, Oct 06, 2012 at 12:47:56PM -0700, ecashin@coraid.com wrote:
> Hi.  I have a function that enters with a lock held and does
> an unlock inside a loop.
> 
> Sparse 0.4.4 is fine with this function until I introduce a conditional
> between the unlock and the next lock.  In the minimal example below,
> changing the "#if 1" to "#if 0" makes sparse generate the warning
> below:
> 
> cd ~/git/linux && PATH=/opt/bin:$PATH make drivers/block/aoe/aoe.ko C=1
> make[1]: Nothing to be done for `all'.
> make[1]: Nothing to be done for `relocs'.
>   CHK     include/linux/version.h
>   CHK     include/generated/utsrelease.h
>   CALL    scripts/checksyscalls.sh
>   CHECK   drivers/block/aoe/demo.c
> /build/ecashin/git/linux/arch/x86/include/asm/spinlock.h:81:9: warning: context imbalance in 'demofn' - unexpected unlock
>   CC [M]  drivers/block/aoe/demo.o
>   LD [M]  drivers/block/aoe/aoe.o
>   MODPOST 1 modules
>   CC      drivers/block/aoe/aoe.mod.o
>   LD [M]  drivers/block/aoe/aoe.ko
> 
> I'm using 3.6.0-rc7 kernel sources.
> 
> Granted, I'm unusually tired today, but I can't think of a way that
> conditionally printing a warning has changed the locking, so I
> could use some help in determining whether this is a sparse bug
> that might be fixed, one that I have to work around, or some
> confusion of mine.
> 
> /* demo.c */
> #include <linux/netdevice.h>
> 
> static spinlock_t lk;
> static struct sk_buff_head q;
> int demofn(void);
> 
> /* enters and returns with lk held */
> int demofn(void)
> {
> 	struct sk_buff *skb;
> 
> 	while ((skb = skb_dequeue(&q))) {
> 		spin_unlock_irq(&lk);
> #if 1
> 		dev_queue_xmit(skb);
> #else
> 		if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
> 			pr_warn("informative warning\n");
> #endif
> 		spin_lock_irq(&lk);
> 	}
> 	return 0;
> }

Sparse should *always* generate a context warning here; odd that it does
not in both cases.

The right fix: annotate the function to explicitly say it starts and
stops with that lock held.  That should make the warning go away in
both cases.

- Josh Triplett

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Ed Cashin]

On Oct 6, 2012, at 4:21 PM, Josh Triplett wrote:

> On Sat, Oct 06, 2012 at 12:47:56PM -0700, ecashin@coraid.com wrote:
...
>> static spinlock_t lk;
>> static struct sk_buff_head q;
>> int demofn(void);
>> 
>> /* enters and returns with lk held */
>> int demofn(void)
>> {
>> 	struct sk_buff *skb;
>> 
>> 	while ((skb = skb_dequeue(&q))) {
>> 		spin_unlock_irq(&lk);
>> #if 1
>> 		dev_queue_xmit(skb);
>> #else
>> 		if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
>> 			pr_warn("informative warning\n");
>> #endif
>> 		spin_lock_irq(&lk);
>> 	}
>> 	return 0;
>> }
> 
> Sparse should *always* generate a context warning here; odd that it does
> not in both cases.

I see.

> The right fix: annotate the function to explicitly say it starts and
> stops with that lock held.  That should make the warning go away in
> both cases.


OK.  From the sparse man page section on context, along with
include/linux/compiler.h, it sounds like the way to do exactly that 
would be something unusual:

int demofn(void) __attribute__((context(&lk,1,1)))

... but using that in demo.c causes sparse to warn me that it's 
ignoring that attribute, so I doubt that can be what you mean.

Were you thinking of changes like the ones below?  These changes
stop the warnings, but it bothers me that they imply that the function
is called without the lock held, __attribute__((context(x,0,1))), when
that's not really true.

[ecashin@marino linux]$ diff -u drivers/block/aoe/demo.c.20121006 drivers/block/aoe/demo.c
--- drivers/block/aoe/demo.c.20121006   2012-10-06 21:12:11.769751545 -0400
+++ drivers/block/aoe/demo.c    2012-10-06 21:51:01.453595477 -0400
@@ -5,10 +5,11 @@
 int demofn(void);
 
 /* enters with lk held */
-int demofn(void)
+int demofn(void) __acquires(&lk)
 {
        struct sk_buff *skb;
 
+       __acquire(lk);
        while ((skb = skb_dequeue(&q))) {
                spin_unlock_irq(&lk);
 #if 0
[ecashin@marino linux]$ 

Thanks very much.

-- 
  Ed Cashin
  ecashin@coraid.com

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Josh Triplett]

On Sat, Oct 06, 2012 at 08:56:57PM -0500, Ed Cashin wrote:
> On Oct 6, 2012, at 4:21 PM, Josh Triplett wrote:
> 
> > On Sat, Oct 06, 2012 at 12:47:56PM -0700, ecashin@coraid.com wrote:
> ...
> >> static spinlock_t lk;
> >> static struct sk_buff_head q;
> >> int demofn(void);
> >> 
> >> /* enters and returns with lk held */
> >> int demofn(void)
> >> {
> >> 	struct sk_buff *skb;
> >> 
> >> 	while ((skb = skb_dequeue(&q))) {
> >> 		spin_unlock_irq(&lk);
> >> #if 1
> >> 		dev_queue_xmit(skb);
> >> #else
> >> 		if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
> >> 			pr_warn("informative warning\n");
> >> #endif
> >> 		spin_lock_irq(&lk);
> >> 	}
> >> 	return 0;
> >> }
> > 
> > Sparse should *always* generate a context warning here; odd that it does
> > not in both cases.
> 
> I see.
> 
> > The right fix: annotate the function to explicitly say it starts and
> > stops with that lock held.  That should make the warning go away in
> > both cases.
> 
> 
> OK.  From the sparse man page section on context, along with
> include/linux/compiler.h, it sounds like the way to do exactly that 
> would be something unusual:
> 
> int demofn(void) __attribute__((context(&lk,1,1)))
> 
> ... but using that in demo.c causes sparse to warn me that it's 
> ignoring that attribute, so I doubt that can be what you mean.

I did mean precisely that; I don't know why Sparse complains about that
syntax.

- Josh Triplett

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Ed Cashin]

On Oct 6, 2012, at 10:39 PM, Josh Triplett wrote:

> On Sat, Oct 06, 2012 at 08:56:57PM -0500, Ed Cashin wrote:
...
>> OK.  From the sparse man page section on context, along with
>> include/linux/compiler.h, it sounds like the way to do exactly that 
>> would be something unusual:
>> 
>> int demofn(void) __attribute__((context(&lk,1,1)))
>> 
>> ... but using that in demo.c causes sparse to warn me that it's 
>> ignoring that attribute, so I doubt that can be what you mean.
> 
> I did mean precisely that; I don't know why Sparse complains about that
> syntax.

Maybe there's a header I need.  Searching with cscope and ctags for definitions of "context" doesn't seem to be the right kind of searching.

The complaint looks like:

  CC [M]  drivers/block/aoe/demo.o
drivers/block/aoe/demo.c:9: warning: `context' attribute directive ignored
drivers/block/aoe/demo.c:9: error: expected `,' or `;' before `{' token
make[1]: *** [drivers/block/aoe/demo.o] Error 1
make: *** [drivers/block/aoe/aoe.ko] Error 2

... for this code:

     1  #include <linux/netdevice.h>
     2  
     3  static spinlock_t lk;
     4  static struct sk_buff_head q;
     5  int demofn(void);
     6  
     7  /* enters with lk held */
     8  int demofn(void) __attribute__((context(&lk,1,1)))
     9  {
    10          struct sk_buff *skb;
    11  
    12          while ((skb = skb_dequeue(&q))) {
    13                  spin_unlock_irq(&lk);
    14                  if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
    15                          pr_warn("informative warning\n");
    16                  spin_lock_irq(&lk);
    17          }
    18          return 0;
    19  }

Thanks.

-- 
  Ed Cashin
  ecashin@coraid.com

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Josh Triplett]

On Sun, Oct 07, 2012 at 07:49:25AM -0500, Ed Cashin wrote:
> On Oct 6, 2012, at 10:39 PM, Josh Triplett wrote:
> 
> > On Sat, Oct 06, 2012 at 08:56:57PM -0500, Ed Cashin wrote:
> ...
> >> OK.  From the sparse man page section on context, along with
> >> include/linux/compiler.h, it sounds like the way to do exactly that 
> >> would be something unusual:
> >> 
> >> int demofn(void) __attribute__((context(&lk,1,1)))
> >> 
> >> ... but using that in demo.c causes sparse to warn me that it's 
> >> ignoring that attribute, so I doubt that can be what you mean.
> > 
> > I did mean precisely that; I don't know why Sparse complains about that
> > syntax.
> 
> Maybe there's a header I need.  Searching with cscope and ctags for definitions of "context" doesn't seem to be the right kind of searching.
> 
> The complaint looks like:
> 
>   CC [M]  drivers/block/aoe/demo.o
> drivers/block/aoe/demo.c:9: warning: `context' attribute directive ignored
> drivers/block/aoe/demo.c:9: error: expected `,' or `;' before `{' token
> make[1]: *** [drivers/block/aoe/demo.o] Error 1
> make: *** [drivers/block/aoe/aoe.ko] Error 2

Oh, that complaint doesn't come from Sparse; that comes from GCC, since
GCC doesn't understand the context attribute.  Look at
include/linux/compiler.h; it has wrapper macros for the various Sparse
attributes, and defines them to nothing when not compiling with Sparse.
If you want to use the context attribute to denote a lock held through a
function, you need a patch like this:

From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
From: Josh Triplett <josh@joshtriplett.org>
Date: Sun, 7 Oct 2012 12:41:13 -0700
Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held

linux/compiler.h has macros to denote functions that acquire or release
locks, but not to denote functions called with a lock held that return
with the lock still held.  Add a __must_hold macro to cover this case.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/linux/compiler.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index f430e41..b121554 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -10,6 +10,7 @@
 # define __force	__attribute__((force))
 # define __nocast	__attribute__((nocast))
 # define __iomem	__attribute__((noderef, address_space(2)))
+# define __must_hold(x)	__attribute__((context(x,1,1)))
 # define __acquires(x)	__attribute__((context(x,0,1)))
 # define __releases(x)	__attribute__((context(x,1,0)))
 # define __acquire(x)	__context__(x,1)
@@ -33,6 +34,7 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 # define __chk_user_ptr(x) (void)0
 # define __chk_io_ptr(x) (void)0
 # define __builtin_warning(x, y...) (1)
+# define __must_hold(x)
 # define __acquires(x)
 # define __releases(x)
 # define __acquire(x) (void)0

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Ed Cashin]

On Oct 7, 2012, at 3:45 PM, Josh Triplett wrote:

> On Sun, Oct 07, 2012 at 07:49:25AM -0500, Ed Cashin wrote:
...
>>  CC [M]  drivers/block/aoe/demo.o
>> drivers/block/aoe/demo.c:9: warning: `context' attribute directive ignored
>> drivers/block/aoe/demo.c:9: error: expected `,' or `;' before `{' token
>> make[1]: *** [drivers/block/aoe/demo.o] Error 1
>> make: *** [drivers/block/aoe/aoe.ko] Error 2
> 
> Oh, that complaint doesn't come from Sparse; that comes from GCC, since
> GCC doesn't understand the context attribute.  Look at
> include/linux/compiler.h; it has wrapper macros for the various Sparse
> attributes, and defines them to nothing when not compiling with Sparse.
> If you want to use the context attribute to denote a lock held through a
> function, you need a patch like this:
> 
> From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
> From: Josh Triplett <josh@joshtriplett.org>
> Date: Sun, 7 Oct 2012 12:41:13 -0700
> Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held

Ah.  OK.  So... would you like me to submit your patch to the LKML, or are you doing that?

It seems like a needed addition.

> linux/compiler.h has macros to denote functions that acquire or release
> locks, but not to denote functions called with a lock held that return
> with the lock still held.  Add a __must_hold macro to cover this case.
> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
> include/linux/compiler.h |    2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index f430e41..b121554 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -10,6 +10,7 @@
> # define __force	__attribute__((force))
> # define __nocast	__attribute__((nocast))
> # define __iomem	__attribute__((noderef, address_space(2)))
> +# define __must_hold(x)	__attribute__((context(x,1,1)))
> # define __acquires(x)	__attribute__((context(x,0,1)))
> # define __releases(x)	__attribute__((context(x,1,0)))
> # define __acquire(x)	__context__(x,1)
> @@ -33,6 +34,7 @@ extern void __chk_io_ptr(const volatile void __iomem *);
> # define __chk_user_ptr(x) (void)0
> # define __chk_io_ptr(x) (void)0
> # define __builtin_warning(x, y...) (1)
> +# define __must_hold(x)
> # define __acquires(x)
> # define __releases(x)
> # define __acquire(x) (void)0

-- 
  Ed Cashin
  ecashin@coraid.com

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Josh Triplett]

On Sun, Oct 07, 2012 at 04:28:16PM -0500, Ed Cashin wrote:
> On Oct 7, 2012, at 3:45 PM, Josh Triplett wrote:
> 
> > On Sun, Oct 07, 2012 at 07:49:25AM -0500, Ed Cashin wrote:
> ...
> >>  CC [M]  drivers/block/aoe/demo.o
> >> drivers/block/aoe/demo.c:9: warning: `context' attribute directive ignored
> >> drivers/block/aoe/demo.c:9: error: expected `,' or `;' before `{' token
> >> make[1]: *** [drivers/block/aoe/demo.o] Error 1
> >> make: *** [drivers/block/aoe/aoe.ko] Error 2
> > 
> > Oh, that complaint doesn't come from Sparse; that comes from GCC, since
> > GCC doesn't understand the context attribute.  Look at
> > include/linux/compiler.h; it has wrapper macros for the various Sparse
> > attributes, and defines them to nothing when not compiling with Sparse.
> > If you want to use the context attribute to denote a lock held through a
> > function, you need a patch like this:
> > 
> > From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
> > From: Josh Triplett <josh@joshtriplett.org>
> > Date: Sun, 7 Oct 2012 12:41:13 -0700
> > Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held
> 
> Ah.  OK.  So... would you like me to submit your patch to the LKML, or are you doing that?
> 
> It seems like a needed addition.

If you can confirm that it works for you, I'll send it to LKML with a
Tested-by from you.

- Josh Triplett

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Ed Cashin]

On Oct 7, 2012, at 7:30 PM, Josh Triplett wrote:

> On Sun, Oct 07, 2012 at 04:28:16PM -0500, Ed Cashin wrote:
>> On Oct 7, 2012, at 3:45 PM, Josh Triplett wrote:
...
>>> From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
>>> From: Josh Triplett <josh@joshtriplett.org>
>>> Date: Sun, 7 Oct 2012 12:41:13 -0700
>>> Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held
>> 
>> Ah.  OK.  So... would you like me to submit your patch to the LKML, or are you doing that?
>> 
>> It seems like a needed addition.
> 
> If you can confirm that it works for you, I'll send it to LKML with a
> Tested-by from you.

Does my usage in demo.c (as shown below) look correct to you?  If so, then I can confirm that it eliminates the warnings as intended.

Similar usage in the original code that motivated this inquiry also passes sparse when using __must_hold() with your patch applied.

[ecashin@marino linux]$ nl -b a drivers/block/aoe/demo.c
     1  #include <linux/netdevice.h>
     2  #include <linux/compiler.h>
     3  
     4  static spinlock_t lk;
     5  static struct sk_buff_head q;
     6  int demofn(void);
     7  
     8  /* enters with lk held */
     9  int demofn(void) __must_hold(&lk)
    10  {
    11          struct sk_buff *skb;
    12  
    13          while ((skb = skb_dequeue(&q))) {
    14                  spin_unlock_irq(&lk);
    15                  if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
    16                          pr_warn("informative warning\n");
    17                  spin_lock_irq(&lk);
    18          }
    19          return 0;
    20  }
[ecashin@marino linux]$ 

Thanks much!

-- 
  Ed Cashin
  ecashin@coraid.com

Re: "unexpected unlock" when unlocking, conditional, lock in loop

[Josh Triplett]

On Sun, Oct 07, 2012 at 07:35:44PM -0500, Ed Cashin wrote:
> On Oct 7, 2012, at 7:30 PM, Josh Triplett wrote:
> 
> > On Sun, Oct 07, 2012 at 04:28:16PM -0500, Ed Cashin wrote:
> >> On Oct 7, 2012, at 3:45 PM, Josh Triplett wrote:
> ...
> >>> From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
> >>> From: Josh Triplett <josh@joshtriplett.org>
> >>> Date: Sun, 7 Oct 2012 12:41:13 -0700
> >>> Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held
> >> 
> >> Ah.  OK.  So... would you like me to submit your patch to the LKML, or are you doing that?
> >> 
> >> It seems like a needed addition.
> > 
> > If you can confirm that it works for you, I'll send it to LKML with a
> > Tested-by from you.
> 
> Does my usage in demo.c (as shown below) look correct to you?  If so, then I can confirm that it eliminates the warnings as intended.
> 
> Similar usage in the original code that motivated this inquiry also passes sparse when using __must_hold() with your patch applied.
> 
> [ecashin@marino linux]$ nl -b a drivers/block/aoe/demo.c
>      1  #include <linux/netdevice.h>
>      2  #include <linux/compiler.h>
>      3  
>      4  static spinlock_t lk;
>      5  static struct sk_buff_head q;
>      6  int demofn(void);
>      7  
>      8  /* enters with lk held */
>      9  int demofn(void) __must_hold(&lk)
>     10  {
>     11          struct sk_buff *skb;
>     12  
>     13          while ((skb = skb_dequeue(&q))) {
>     14                  spin_unlock_irq(&lk);
>     15                  if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
>     16                          pr_warn("informative warning\n");
>     17                  spin_lock_irq(&lk);
>     18          }
>     19          return 0;
>     20  }
> [ecashin@marino linux]$ 

Yes, that looks right to me; glad to hear it works for you.  I'll send
the patch to LKML shortly.

Meanwhile, you've still found at least one bug in Sparse, since it
doesn't give the context warning if you leave out lines 15 and 16.

- Josh Triplett