nbd: fix race conditions

This patch is similar to one I posted yesterday for 2.6. It fixes the following race conditions in nbd:

1) adds locking and properly orders the code in NBD_CLEAR_SOCK to eliminate races with other code

2) adds an lo->sock check to nbd_clear_que to eliminate races between do_nbd_request and nbd_clear_que, which resulted in the dequeuing of active requests

3) adds an lo->sock check to NBD_DO_IT to eliminate races with NBD_CLEAR_SOCK, which caused an Oops when "nbd-client -d" was called

drivers/block/nbd.c +0 -0

 	case NBD_SET_SOCK:
 		if (lo->file)
 			return -EBUSY;

@@ -491,9 +492,12 @@ static int nbd_ioctl(struct inode *inode
 		 * there should be a more generic interface rather than
 		 * calling socket ops directly here */

drivers/block/nbd.c +0 -0

 		up(&lo->tx_lock);
 		spin_lock(&lo->queue_lock);
 		file = lo->file;

Quoted 5 lines · In article <[email protected]> you wrote:

This patch is similar to one I posted yesterday for 2.6. It fixes the following race conditions in nbd:

1) adds locking and properly orders the code in NBD_CLEAR_SOCK to eliminate races with other code

This is not so clear as for the 2.6 code.

There's no ref_count in the
2.4 request struct, so you have no standard way to mark a request as in-flight, and hence no standard way to get clr_queue to skip it for the

Quoted 7 lines

2) adds an lo->sock check to nbd_clear_que to eliminate races between do_nbd_request and nbd_clear_que, which resulted in the dequeuing of active requests

3) adds an lo->sock check to NBD_DO_IT to eliminate races with NBD_CLEAR_SOCK, which caused an Oops when "nbd-client -d" was called

All these are for the same thing - the metadataraces.

Review

Quoted 7 lines

(quoted diff) +2 -4

-             file = lo->file;
-             if (!file) {
-                     spin_unlock(&lo->queue_lock);
-                     return -EINVAL;
+                     printk(KERN_ERR "nbd: disconnect: some requests are in progress -> please try again.\n");
+                     error = -EBUSY;
              }

... it looks as though if the queue is nonempty, we now still return busy. So what you did was remove the test on lo->file. Now we don't care about the lo->file state. Why?

Review

Quoted 8 lines

case NBD_CLEAR_QUE:

(quoted diff) +7 -0

+             down(&lo->tx_lock);
+             if (lo->sock) {
+                     up(&lo->tx_lock);
+                     return 0; /* probably should be error, but that would
+                                * break "nbd-client -d", so just return 0 */
+             }
+             up(&lo->tx_lock);

You don't clear queue while lo->sock is nonzero. This means that one must have cleared socket beforehand.

I don't see any race conditions going away. Except possibly a race between set sock and clear sock to set lo->sock. The setting of lo->file is also a little more atomic now, and that's OK. But surely the race condition you cured in 2.6 was not this at all, but a race between clearing the queue and doing the requests?

3 of 4 race conditions that were fixed in the 2.6 patch are fixed here. As I said above, fixing the 4th is not simple, and may not be worth it at this point for 2.4.