Re: wrong "imbalanced unlock" warning?

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

On Mon, Jan 29, 2018 at 05:32:53PM +0100, Aurélien Aptel wrote:
> Hi,
> 
> While compiling the kernel cifs for-next (from here [1]) I've run into a
> strange sparse warning.
> 
> fs/cifs/smbdirect.c:1943:56: warning: context imbalance in 'smbd_recv_buf' - unexpected unlock
> 
> The code is basically doing:
> 
> 	int queue_length;
> 	....
> 	queue_length = info->reassembly_queue_length;
> 	....
> 	/*
> 	 * No need to lock if we are not at the
> 	 * end of the queue
> 	 */
> 	if (!queue_length)
> 	    spin_lock_irq(&info->reassembly_queue_lock);
> 	list_del(&response->list);
> 	queue_removed++;
> 	if (!queue_length)
> 	    spin_unlock_irq(&info->reassembly_queue_lock);
> 
> 
> I don't think there's anything wrong with this code.

Indeed.
Sparse issues a "context imbalance" warning if, at some point,
the code can be reached via two paths (or more) and the lock state
(the context) is not identical in each of these paths.

It's exactly what's happening in this function.
It can be annoying or seen as a limitation of sparse context
checking, on the other hand it's often what is wanted.


> I've tried adding a
> test in sparse validation/context.c to reproduce the warning:
> 
> add this at the bottom:
> 
>     static void warn_cifs(void)
>     {
> 
>         int x;
>         x = condition;
> 
>         if(!x)
>             a();
> 
>         if(!x)
>             r();
>     }
> 
> a() adds a lock, r() removes it.
> 
> and in validation/ run
> 
>     ./test-suite single context.c
>          TEST    Check -Wcontext (context.c)
>     context.c passed !

That's normal, because the code is very simple and sparse can see that
the second test is redundant.
If you add something between the two tests & lock/unlock something
that can change the condition then you will have the same behaviour
as the kernel code.

> I've played around with the test and tried to make it similar to
> smbdirect.c code (see attached file). Place it in validation and run
> ./test-suite single cifs.c
> 
> I've also checked out my system revision of sparse, but I couldn't make
> it fail either.
> 
> Although it doesn't look like it, maybe there is an issue in
> smbdirect.c, what do you think?

Well, now you have the explanation for sparse's warning but
looking only at the small part you copied, I have directly
several questions about the locking like, for example:
 - is it valid to deref info->reassembly_queue_length
   before taking the lock?
or equivalently:
 - is it possible that something is added to the queue
   after initializing queue_length but before the lock
   is taken?

I hope this answer your questions,
-- Luc Van Oostenryck