staging: New driver: Xillybus generic interface for FPGA

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

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

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

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

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