wrong "imbalanced unlock" warning?

wrong "imbalanced unlock" warning?

[Aurélien Aptel]

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

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. 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 !

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?

1: git://git.samba.org/sfrench/cifs-2.6.git tip is 36c7ce4a17f2 as of
   now

   make sure to enable INFINIBAND, CIFS and CIFS_SMB_DIRECT


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cifs.c --]
[-- Type: text/x-csrc, Size: 679 bytes --]


static void lock(void) __attribute__((context(0,1)))
{
	__context__(1);
}

static void unlock(void) __attribute__((context(1,0)))
{
	__context__(-1);
}

extern int cond, cond2;

static void foo (void)
{
}

static void warn_cifs(void)
{

again:
	if (cond) {
		return;
	}

	if (cond) {
		int x;

		x = cond2;

		while (cond) {
			if (cond) {
				goto read_rfc1002_done;
			}

			if (cond) {
				x--;
				if (!x)
					lock();
				foo();
				if (!x)
					unlock();

			} else
				foo();

		}

		lock();
		foo();
		unlock();

read_rfc1002_done:
		return;
	}


	if (cond)
		return;

	goto again;
}

/*
 * check-name: Check -Wcontext bis
 *
 * check-error-start
 * check-error-end
 */

[-- Attachment #3: Type: text/plain, Size: 245 bytes --]


-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: wrong "imbalanced unlock" warning?

[Luc Van Oostenryck]

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