[bug report] staging: New driver: Xillybus generic interface for FPGA

[bug report] staging: New driver: Xillybus generic interface for FPGA

[Dan Carpenter]

Hello Eli Billauer,

The patch 48bae0507410: "staging: New driver: Xillybus generic
interface for FPGA" from Jun 24, 2013, leads to the following static
checker warning:

	drivers/char/xillybus/xillybus_core.c:941 xillybus_read()
	error: double unlock 'mutex:&channel->wr_mutex'

drivers/char/xillybus/xillybus_core.c
   904  
   905                                  if (mutex_lock_interruptible(
   906                                              &channel->wr_mutex))
   907                                          goto interrupted;
                                                ^^^^^^^^^^^^^^^^
We were interrupted before we could take the lock.

   908                          } while (channel->wr_sleepy);
   909  
   910                          continue;
   911  
   912  interrupted: /* Mutex is not held if got here */
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Comment agrees.

   913                          if (channel->endpoint->fatal_error)
   914                                  return -EIO;
   915                          if (bytes_done)
   916                                  return bytes_done;
   917                          if (filp->f_flags & O_NONBLOCK)
   918                                  return -EAGAIN; /* Don't admit snoozing */
   919                          return -EINTR;
   920                  }
   921  
   922                  left_to_sleep = deadline - ((long) jiffies);
   923  
   924                  /*
   925                   * If our time is out, skip the waiting. We may miss wr_sleepy
   926                   * being deasserted but hey, almost missing the train is like
   927                   * missing it.
   928                   */
   929  
   930                  if (left_to_sleep > 0) {
   931                          left_to_sleep    932                                  wait_event_interruptible_timeout(
   933                                          channel->wr_wait,
   934                                          (!channel->wr_sleepy),
   935                                          left_to_sleep);
   936  
   937                          if (left_to_sleep > 0) /* wr_sleepy deasserted */
   938                                  continue;
   939  
   940                          if (left_to_sleep < 0) { /* Interrupt */
   941                                  mutex_unlock(&channel->wr_mutex);
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The static checker thinks we still don't have the lock sio it complains.
Possibly a false positive though because of the wait earlier?

   942                                  if (channel->endpoint->fatal_error)
   943                                          return -EIO;
   944                                  if (bytes_done)
   945                                          return bytes_done;
   946                                  return -EINTR;
   947                          }
   948                  }

regards,
dan carpenter

Re: [bug report] staging: New driver: Xillybus generic interface for FPGA

[Eli Billauer]

Hello Dan,

Thanks for reviewing the driver.

I've taken a look on the code. Since your line numbering and path match 
xillybus_core.c current state, I relate to that, and not to the said patch.

This seems like a false positive to me: Throughout the relevant function 
(xillybus_read), the unlock of wr_mutex is always followed by a return 
command, except for the do-loop, which is partly cited in your message. 
As for the latter, there are only two ways out of that loop: Either 
return immediately (via the "interrupted" label) or re-acquire the mutex.

So again, not having the mutex locked (outside that do-loop) means a 
return command coming very soon.

This makes me conclude that a double unlock isn't possible in this function.

I can't say much on why the static checker raised this error. Maybe 
because it didn't detect that reaching "interrupted" means a certain 
return command?

Regards,
    Eli

On 12/10/16 11:28, Dan Carpenter wrote:
> Hello Eli Billauer,
>
> The patch 48bae0507410: "staging: New driver: Xillybus generic
> interface for FPGA" from Jun 24, 2013, leads to the following static
> checker warning:
>
> 	drivers/char/xillybus/xillybus_core.c:941 xillybus_read()
> 	error: double unlock 'mutex:&channel->wr_mutex'
>
> drivers/char/xillybus/xillybus_core.c
>     904
>     905                                  if (mutex_lock_interruptible(
>     906&channel->wr_mutex))
>     907                                          goto interrupted;
>                                                  ^^^^^^^^^^^^^^^^
> We were interrupted before we could take the lock.
>
>     908                          } while (channel->wr_sleepy);
>     909
>     910                          continue;
>     911
>     912  interrupted: /* Mutex is not held if got here */
>                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Comment agrees.
>
>     913                          if (channel->endpoint->fatal_error)
>     914                                  return -EIO;
>     915                          if (bytes_done)
>     916                                  return bytes_done;
>     917                          if (filp->f_flags&  O_NONBLOCK)
>     918                                  return -EAGAIN; /* Don't admit snoozing */
>     919                          return -EINTR;
>     920                  }
>     921
>     922                  left_to_sleep = deadline - ((long) jiffies);
>     923
>     924                  /*
>     925                   * If our time is out, skip the waiting. We may miss wr_sleepy
>     926                   * being deasserted but hey, almost missing the train is like
>     927                   * missing it.
>     928                   */
>     929
>     930                  if (left_to_sleep>  0) {
>     931                          left_to_sleep >     932                                  wait_event_interruptible_timeout(
>     933                                          channel->wr_wait,
>     934                                          (!channel->wr_sleepy),
>     935                                          left_to_sleep);
>     936
>     937                          if (left_to_sleep>  0) /* wr_sleepy deasserted */
>     938                                  continue;
>     939
>     940                          if (left_to_sleep<  0) { /* Interrupt */
>     941                                  mutex_unlock(&channel->wr_mutex);
>                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The static checker thinks we still don't have the lock sio it complains.
> Possibly a false positive though because of the wait earlier?
>
>     942                                  if (channel->endpoint->fatal_error)
>     943                                          return -EIO;
>     944                                  if (bytes_done)
>     945                                          return bytes_done;
>     946                                  return -EINTR;
>     947                          }
>     948                  }
>
> regards,
> dan carpenter
>
>    

Re: [bug report] staging: New driver: Xillybus generic interface for FPGA

[Dan Carpenter]

I have read and re-read your repsonse and I can't understand it at all.

The only way this code is correct is if we take the lock when we do:

   930                  if (left_to_sleep > 0) {

Here we are unlocked.

   931                          left_to_sleep    932                                  wait_event_interruptible_timeout(
   933                                          channel->wr_wait,
   934                                          (!channel->wr_sleepy),
   935                                          left_to_sleep);

Are you saying that here we are locked?  That does NOT appear to be what
you are saying, but it is the only response that I would accept to say
that the code is correct.

In other words, the bug is probably real.  Please review again.

regards,
dan carpenter

Re: [bug report] staging: New driver: Xillybus generic interface for FPGA

[Eli Billauer]

Hello Dan,

Sorry, it seems like I didn't make myself clear. I'll try again, in more 
detail.

The purpose and principle of channel->wr_mutex is that it's locked as 
long as the relevant function, xillybus_read(), is invoked. Except for 
when waiting for channel->wr_wait, in which case the mutex is unlocked 
for the sake of sleeping, and then locked after being waken up.

In detail: The mutex is locked soon after the function is invoked:

690         rc = mutex_lock_interruptible(&channel->wr_mutex);
691         if (rc)
692                 return rc;

and unlocked only just before returning throughout the function, e.g.

788                         if (rc) {
789                                 mutex_unlock(&channel->wr_mutex);
790                                 return rc;
791                         }

And then there's the temporary unlock, which triggered the static checker:

897                         do {
898                                 mutex_unlock(&channel->wr_mutex);
899
900                                 if (wait_event_interruptible(
901                                             channel->wr_wait,
902                                             (!channel->wr_sleepy)))
903                                         goto interrupted;
904
905                                 if (mutex_lock_interruptible(
906 &channel->wr_mutex))
907                                         goto interrupted;
908                         } while (channel->wr_sleepy);

The mutex is already locked when the loop is reached. In the loop, it's 
unlocked for the sake of sleeping, and then locked when waking up. 
Signal handling makes it a bit more tangled than that, but the principle 
of the wr_mutex remains, because there are only two ways to get out of 
this do-loop:

(1) By successfully locking the mutex in line 905 (and the while 
condition being false)
(2) By jumping to "interrupted", in which case the mutex is indeed 
unlocked, but the function returns in that case, and in particular 
doesn't reach line 930.

So yes, I'm saying that the mutex is surely locked when reaching line 
930. It's difficult to check all paths of execution, but I stick to the 
principle: The mutex remains locked as long as the function is invoked 
(except for just after invocation, or just before returning, or within 
the do-loop at 897-908).

Do you see any execution path for which this principle is broken? Is 
there any execution path that brings us to line 930, for example, with 
the mutex unlocked? I can't find one. If there is such a path, there's a 
bug indeed.

I hope this clarified my view. If I've missed something, by all means 
please pinpoint my mistake.

Thanks,
    Eli

On 17/10/16 14:40, Dan Carpenter wrote:
> I have read and re-read your repsonse and I can't understand it at all.
>
> The only way this code is correct is if we take the lock when we do:
>
>     930                  if (left_to_sleep>  0) {
>
> Here we are unlocked.
>
>     931                          left_to_sleep >     932                                  wait_event_interruptible_timeout(
>     933                                          channel->wr_wait,
>     934                                          (!channel->wr_sleepy),
>     935                                          left_to_sleep);
>
> Are you saying that here we are locked?  That does NOT appear to be what
> you are saying, but it is the only response that I would accept to say
> that the code is correct.
>
> In other words, the bug is probably real.  Please review again.
>
> regards,
> dan carpenter
>
>    

Re: [bug report] staging: New driver: Xillybus generic interface for FPGA

[Dan Carpenter]

Ah...  Great.  Thanks.  That was my mistake.  I misread the code.

I seem to have introduced a flow analysis bug in my devel version of
Smatch.  Hopefully, I didn't push it...  I just had a harddrive fail
recently so everything is a bit of a mess for me right now but I will
investigate why this false positive happened.

regards,
dan carpenter