Re: + nbd-fix-tx-rx-race-condition.patch added to -mm tree

Re: + nbd-fix-tx-rx-race-condition.patch added to -mm tree

[Herbert Xu]

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

On Sat, Nov 19, 2005 at 11:02:13AM -0500, Paul Clements wrote:
> 
> Thanks for this. It looks correct, for the most part, and seems to deal 
> with the various race conditions well (probably more cleanly than we 
> were doing before). There is one problem with this patch, though. You 
> have removed the acquiring of queue_lock from nbd_clear_que. I don't 
> know if that was intentional or not, but the queue_lock must be taken 
> when the queue is being modified. For instance, if nbd-client -d 
> (disconnect) gets called on an active nbd device, then you'll have a 
> race between requests being completed and pulled off the queue 
> (nbd_find_request) and nbd-client -d (which calls nbd_clear_que).

This is intentional actually.  nbd_clear_queue never races against
nbd_find_request because the ioctl is protected by the BKL.  If it
weren't, then we have much worse problems to worry about (e.g.,
while you're clearing the queue someone else could have set the
socket again and started queueing requests).

> >+	/*
> >+	 * lo->sock == NULL guarantees that the queue will not grow beyond 
> >the
> >+	 * current active request.
> >+	 */

This comment is why we can do without the locking.  The only other
function that modifies the list (apart from nbd_find_request and
nbd_clear_queueis) is nbd_do_request.

When we enter nbd_clear_queue, lo->sock must have been set to NULL.
In fact, we always take the tx_lock while setting lo->sock.  So we
have two possibilities:

CPU0						CPU1
						nbd_do_request
down(tx_lock)
lo->sock = NULL
up(tx_lock)
							down(tx_lock)
							bail as lo->sock is
								NULL
							up(tx_lock)
	nbd_clear_queue

If any request occurs after the setting, then clearly the list will not
be modified.


CPU0						CPU1
						nbd_do_request
							down(tx_lock)
							add req to list
							up(tx_lock)
down(tx_lock)
lo->sock = NULL
up(tx_lock)
	nbd_clear_queue

If any request occurs before the setting, the list has been modified
already.  The up/down also gives us the required memory barriers so
that we can operate on the list safely.

In fact, I just realised that for the same reason active_req must be
NULL as well.  So here is an updated patch which removes the active_req
wait in nbd_clear_queue and the associated memory barrier.

I've also clarified this in the comment.


[NBD] Fix TX/RX race condition

Janos Haar of First NetCenter Bt.  reported numerous crashes involving the
NBD driver.  With his help, this was tracked down to bogus bio vectors
which in turn was the result of a race condition between the
receive/transmit routines in the NBD driver.

The bug manifests itself like this:

CPU0                            CPU1
do_nbd_request
        add req to queuelist
        nbd_send_request
                send req head
                for each bio
                        kmap
                        send
                                nbd_read_stat
                                        nbd_find_request
                                        nbd_end_request
                        kunmap

When CPU1 finishes nbd_end_request, the request and all its associated
bio's are freed.  So when CPU0 calls kunmap whose argument is derived from
the last bio, it may crash.

Under normal circumstances, the race occurs only on the last bio.  However,
if an error is encountered on the remote NBD server (such as an incorrect
magic number in the request), or if there were a bug in the server, it is
possible for the nbd_end_request to occur any time after the request's
addition to the queuelist.

The following patch fixes this problem by making sure that requests are not
added to the queuelist until after they have been completed transmission.

In order for the receiving side to be ready for responses involving
requests still being transmitted, the patch introduces the concept of the
active request.

When a response matches the current active request, its processing is
delayed until after the tranmission has come to a stop.

This has been tested by Janos and it has been successful in curing this
race condition.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 7237 bytes --]

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -54,11 +54,15 @@
 #include <linux/errno.h>
 #include <linux/file.h>
 #include <linux/ioctl.h>
+#include <linux/compiler.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
 #include <net/sock.h>
 
 #include <linux/devfs_fs_kernel.h>
 
 #include <asm/uaccess.h>
+#include <asm/system.h>
 #include <asm/types.h>
 
 #include <linux/nbd.h>
@@ -230,14 +234,6 @@ static int nbd_send_req(struct nbd_devic
 	request.len = htonl(size);
 	memcpy(request.handle, &req, sizeof(req));
 
-	down(&lo->tx_lock);
-
-	if (!sock || !lo->sock) {
-		printk(KERN_ERR "%s: Attempted send on closed socket\n",
-				lo->disk->disk_name);
-		goto error_out;
-	}
-
 	dprintk(DBG_TX, "%s: request %p: sending control (%s@%llu,%luB)\n",
 			lo->disk->disk_name, req,
 			nbdcmd_to_ascii(nbd_cmd(req)),
@@ -276,11 +272,9 @@ static int nbd_send_req(struct nbd_devic
 			}
 		}
 	}
-	up(&lo->tx_lock);
 	return 0;
 
 error_out:
-	up(&lo->tx_lock);
 	return 1;
 }
 
@@ -289,9 +283,14 @@ static struct request *nbd_find_request(
 	struct request *req;
 	struct list_head *tmp;
 	struct request *xreq;
+	int err;
 
 	memcpy(&xreq, handle, sizeof(xreq));
 
+	err = wait_event_interruptible(lo->active_wq, lo->active_req != xreq);
+	if (unlikely(err))
+		goto out;
+
 	spin_lock(&lo->queue_lock);
 	list_for_each(tmp, &lo->queue_head) {
 		req = list_entry(tmp, struct request, queuelist);
@@ -302,7 +301,11 @@ static struct request *nbd_find_request(
 		return req;
 	}
 	spin_unlock(&lo->queue_lock);
-	return NULL;
+
+	err = -ENOENT;
+
+out:
+	return ERR_PTR(err);
 }
 
 static inline int sock_recv_bvec(struct socket *sock, struct bio_vec *bvec)
@@ -331,7 +334,11 @@ static struct request *nbd_read_stat(str
 		goto harderror;
 	}
 	req = nbd_find_request(lo, reply.handle);
-	if (req == NULL) {
+	if (unlikely(IS_ERR(req))) {
+		result = PTR_ERR(req);
+		if (result != -ENOENT)
+			goto harderror;
+
 		printk(KERN_ERR "%s: Unexpected reply (%p)\n",
 				lo->disk->disk_name, reply.handle);
 		result = -EBADR;
@@ -395,19 +402,24 @@ static void nbd_clear_que(struct nbd_dev
 
 	BUG_ON(lo->magic != LO_MAGIC);
 
-	do {
-		req = NULL;
-		spin_lock(&lo->queue_lock);
-		if (!list_empty(&lo->queue_head)) {
-			req = list_entry(lo->queue_head.next, struct request, queuelist);
-			list_del_init(&req->queuelist);
-		}
-		spin_unlock(&lo->queue_lock);
-		if (req) {
-			req->errors++;
-			nbd_end_request(req);
-		}
-	} while (req);
+	/*
+	 * Because we have set lo->sock to NULL under the tx_lock, all
+	 * modifications to the list must have completed by now.  For
+	 * the same reason, the active_req must be NULL.
+	 *
+	 * As a consequence, we don't need to take the spin lock while
+	 * purging the list here.
+	 */
+	BUG_ON(lo->sock);
+	BUG_ON(lo->active_req);
+
+	while (!list_empty(&lo->queue_head)) {
+		req = list_entry(lo->queue_head.next, struct request,
+				 queuelist);
+		list_del_init(&req->queuelist);
+		req->errors++;
+		nbd_end_request(req);
+	}
 }
 
 /*
@@ -435,11 +447,6 @@ static void do_nbd_request(request_queue
 
 		BUG_ON(lo->magic != LO_MAGIC);
 
-		if (!lo->file) {
-			printk(KERN_ERR "%s: Request when not-ready\n",
-					lo->disk->disk_name);
-			goto error_out;
-		}
 		nbd_cmd(req) = NBD_CMD_READ;
 		if (rq_data_dir(req) == WRITE) {
 			nbd_cmd(req) = NBD_CMD_WRITE;
@@ -453,32 +460,34 @@ static void do_nbd_request(request_queue
 		req->errors = 0;
 		spin_unlock_irq(q->queue_lock);
 
-		spin_lock(&lo->queue_lock);
-
-		if (!lo->file) {
-			spin_unlock(&lo->queue_lock);
-			printk(KERN_ERR "%s: failed between accept and semaphore, file lost\n",
-					lo->disk->disk_name);
+		down(&lo->tx_lock);
+		if (unlikely(!lo->sock)) {
+			up(&lo->tx_lock);
+			printk(KERN_ERR "%s: Attempted send on closed socket\n",
+			       lo->disk->disk_name);
 			req->errors++;
 			nbd_end_request(req);
 			spin_lock_irq(q->queue_lock);
 			continue;
 		}
 
-		list_add(&req->queuelist, &lo->queue_head);
-		spin_unlock(&lo->queue_lock);
+		lo->active_req = req;
 
 		if (nbd_send_req(lo, req) != 0) {
 			printk(KERN_ERR "%s: Request send failed\n",
 					lo->disk->disk_name);
-			if (nbd_find_request(lo, (char *)&req) != NULL) {
-				/* we still own req */
-				req->errors++;
-				nbd_end_request(req);
-			} else /* we're racing with nbd_clear_que */
-				printk(KERN_DEBUG "nbd: can't find req\n");
+			req->errors++;
+			nbd_end_request(req);
+		} else {
+			spin_lock(&lo->queue_lock);
+			list_add(&req->queuelist, &lo->queue_head);
+			spin_unlock(&lo->queue_lock);
 		}
 
+		lo->active_req = NULL;
+		up(&lo->tx_lock);
+		wake_up_all(&lo->active_wq);
+
 		spin_lock_irq(q->queue_lock);
 		continue;
 
@@ -529,17 +538,10 @@ static int nbd_ioctl(struct inode *inode
 		down(&lo->tx_lock);
 		lo->sock = NULL;
 		up(&lo->tx_lock);
-		spin_lock(&lo->queue_lock);
 		file = lo->file;
 		lo->file = NULL;
-		spin_unlock(&lo->queue_lock);
 		nbd_clear_que(lo);
-		spin_lock(&lo->queue_lock);
-		if (!list_empty(&lo->queue_head)) {
-			printk(KERN_ERR "nbd: disconnect: some requests are in progress -> please try again.\n");
-			error = -EBUSY;
-		}
-		spin_unlock(&lo->queue_lock);
+		BUG_ON(!list_empty(&lo->queue_head));
 		if (file)
 			fput(file);
 		return error;
@@ -598,24 +600,19 @@ static int nbd_ioctl(struct inode *inode
 			lo->sock = NULL;
 		}
 		up(&lo->tx_lock);
-		spin_lock(&lo->queue_lock);
 		file = lo->file;
 		lo->file = NULL;
-		spin_unlock(&lo->queue_lock);
 		nbd_clear_que(lo);
 		printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
 		if (file)
 			fput(file);
 		return lo->harderror;
 	case NBD_CLEAR_QUE:
-		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);
-		nbd_clear_que(lo);
+		/*
+		 * This is for compatibility only.  The queue is always cleared
+		 * by NBD_DO_IT or NBD_CLEAR_SOCK.
+		 */
+		BUG_ON(!lo->sock && !list_empty(&lo->queue_head));
 		return 0;
 	case NBD_PRINT_DEBUG:
 		printk(KERN_INFO "%s: next = %p, prev = %p, head = %p\n",
@@ -688,6 +685,7 @@ static int __init nbd_init(void)
 		spin_lock_init(&nbd_dev[i].queue_lock);
 		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
 		init_MUTEX(&nbd_dev[i].tx_lock);
+		init_waitqueue_head(&nbd_dev[i].active_wq);
 		nbd_dev[i].blksize = 1024;
 		nbd_dev[i].bytesize = 0x7ffffc00ULL << 10; /* 2TB */
 		disk->major = NBD_MAJOR;
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -37,18 +37,26 @@ enum {
 /* userspace doesn't need the nbd_device structure */
 #ifdef __KERNEL__
 
+#include <linux/wait.h>
+
 /* values for flags field */
 #define NBD_READ_ONLY 0x0001
 #define NBD_WRITE_NOCHK 0x0002
 
+struct request;
+
 struct nbd_device {
 	int flags;
 	int harderror;		/* Code of hard error			*/
 	struct socket * sock;
 	struct file * file; 	/* If == NULL, device is not ready, yet	*/
 	int magic;
+
 	spinlock_t queue_lock;
 	struct list_head queue_head;/* Requests are added here...	*/
+	struct request *active_req;
+	wait_queue_head_t active_wq;
+
 	struct semaphore tx_lock;
 	struct gendisk *disk;
 	int blksize;

[NBD] Use per-device semaphore instead of BKL

[Herbert Xu]

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

On Sun, Nov 20, 2005 at 09:34:19AM +1100, herbert wrote:
> 
> This is intentional actually.  nbd_clear_queue never races against
> nbd_find_request because the ioctl is protected by the BKL.  If it
> weren't, then we have much worse problems to worry about (e.g.,
> while you're clearing the queue someone else could have set the
> socket again and started queueing requests).

Actually, we do have bigger problems :) The BKL is dropped every
time you sleep, and nbd_do_it is definitely a frequent sleeper :)

This isn't really an issue in practice though because the NBD
client program is single-threaded and doesn't share its file
descriptors with anyone else.

However, we shouldn't make it too easy for the user to shoot himself
in the foot.  If he's going to do that, let him at least pay for the
bullet :)

So here is a patch to use a per-device semaphore instead of the
BKL to protect the ioctl's against each other.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 4261 bytes --]

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -500,9 +500,9 @@ error_out:
 	return;
 }
 
-static int nbd_ioctl(struct inode *inode, struct file *file,
-		     unsigned int cmd, unsigned long arg)
+static long nbd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
+	struct inode *inode = file->f_dentry->d_inode;
 	struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
 	int error;
 	struct request sreq ;
@@ -516,6 +516,10 @@ static int nbd_ioctl(struct inode *inode
 	dprintk(DBG_IOCTL, "%s: nbd_ioctl cmd=%s(0x%x) arg=%lu\n",
 			lo->disk->disk_name, ioctl_cmd_to_ascii(cmd), cmd, arg);
 
+	error = down_interruptible(&lo->ioctl_lock);
+	if (unlikely(error))
+		return error;
+
 	switch (cmd) {
 	case NBD_DISCONNECT:
 	        printk(KERN_INFO "%s: NBD_DISCONNECT\n", lo->disk->disk_name);
@@ -528,13 +532,14 @@ static int nbd_ioctl(struct inode *inode
 		 */
 		sreq.sector = 0;
 		sreq.nr_sectors = 0;
+		error = -EINVAL;
                 if (!lo->sock)
-			return -EINVAL;
+			break;
                 nbd_send_req(lo, &sreq);
-                return 0;
+		error = 0;
+		break;
  
 	case NBD_CLEAR_SOCK:
-		error = 0;
 		down(&lo->tx_lock);
 		lo->sock = NULL;
 		up(&lo->tx_lock);
@@ -544,10 +549,12 @@ static int nbd_ioctl(struct inode *inode
 		BUG_ON(!list_empty(&lo->queue_head));
 		if (file)
 			fput(file);
-		return error;
+		break;
+
 	case NBD_SET_SOCK:
+		error = -EBUSY;
 		if (lo->file)
-			return -EBUSY;
+			break;
 		error = -EINVAL;
 		file = fget(arg);
 		if (file) {
@@ -560,29 +567,34 @@ static int nbd_ioctl(struct inode *inode
 				fput(file);
 			}
 		}
-		return error;
+		break;
+
 	case NBD_SET_BLKSIZE:
 		lo->blksize = arg;
 		lo->bytesize &= ~(lo->blksize-1);
 		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_blocksize(inode->i_bdev, lo->blksize);
 		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		break;
+
 	case NBD_SET_SIZE:
 		lo->bytesize = arg & ~(lo->blksize-1);
 		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_blocksize(inode->i_bdev, lo->blksize);
 		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		break;
+
 	case NBD_SET_SIZE_BLOCKS:
 		lo->bytesize = ((u64) arg) * lo->blksize;
 		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_blocksize(inode->i_bdev, lo->blksize);
 		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		break;
+
 	case NBD_DO_IT:
+		error = -EINVAL;
 		if (!lo->file)
-			return -EINVAL;
+			break;
 		nbd_do_it(lo);
 		/* on return tidy up in case we have a signal */
 		/* Forcibly shutdown the socket causing all listeners
@@ -606,28 +618,36 @@ static int nbd_ioctl(struct inode *inode
 		printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
 		if (file)
 			fput(file);
-		return lo->harderror;
+		error = lo->harderror;
+		break;
+
 	case NBD_CLEAR_QUE:
 		/*
 		 * This is for compatibility only.  The queue is always cleared
 		 * by NBD_DO_IT or NBD_CLEAR_SOCK.
 		 */
 		BUG_ON(!lo->sock && !list_empty(&lo->queue_head));
-		return 0;
+		break;
+
 	case NBD_PRINT_DEBUG:
 		printk(KERN_INFO "%s: next = %p, prev = %p, head = %p\n",
 			inode->i_bdev->bd_disk->disk_name,
 			lo->queue_head.next, lo->queue_head.prev,
 			&lo->queue_head);
-		return 0;
+		break;
+
+	default:
+		error = -EINVAL;
+		break;
 	}
-	return -EINVAL;
+
+	up(&lo->ioctl_lock);
+	return error;
 }
 
-static struct block_device_operations nbd_fops =
-{
-	.owner =	THIS_MODULE,
-	.ioctl =	nbd_ioctl,
+static struct block_device_operations nbd_fops = {
+	.owner = THIS_MODULE,
+	.unlocked_ioctl = nbd_ioctl,
 };
 
 /*
@@ -685,6 +705,7 @@ static int __init nbd_init(void)
 		spin_lock_init(&nbd_dev[i].queue_lock);
 		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
 		init_MUTEX(&nbd_dev[i].tx_lock);
+		init_MUTEX(&nbd_dev[i].ioctl_lock);
 		init_waitqueue_head(&nbd_dev[i].active_wq);
 		nbd_dev[i].blksize = 1024;
 		nbd_dev[i].bytesize = 0x7ffffc00ULL << 10; /* 2TB */
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -58,6 +58,8 @@ struct nbd_device {
 	wait_queue_head_t active_wq;
 
 	struct semaphore tx_lock;
+	struct semaphore ioctl_lock;
+
 	struct gendisk *disk;
 	int blksize;
 	u64 bytesize;

Re: [NBD] Use per-device semaphore instead of BKL

[Paul Clements]

Herbert Xu wrote:
> On Sun, Nov 20, 2005 at 09:34:19AM +1100, herbert wrote:
> 
>>This is intentional actually.  nbd_clear_queue never races against
>>nbd_find_request because the ioctl is protected by the BKL.  If it
>>weren't, then we have much worse problems to worry about (e.g.,
>>while you're clearing the queue someone else could have set the
>>socket again and started queueing requests).
> 
> 
> Actually, we do have bigger problems :) The BKL is dropped every
> time you sleep, and nbd_do_it is definitely a frequent sleeper :)

The dropping of the lock in nbd_do_it is actually critical to the way 
nbd functions. nbd_do_it runs for the lifetime of the nbd device, so if 
nbd_do_it were holding some lock (BKL or otherwise), we'd have big problems.


> This isn't really an issue in practice though because the NBD
> client program is single-threaded and doesn't share its file
> descriptors with anyone else.

Right, there's no problem in practice.


> However, we shouldn't make it too easy for the user to shoot himself
> in the foot.  If he's going to do that, let him at least pay for the
> bullet :)
> 
> So here is a patch to use a per-device semaphore instead of the
> BKL to protect the ioctl's against each other.

The problem with this patch is that no ioctls can come in once nbd_do_it 
starts because nbd_do_it runs for the lifetime of the device.

I think we really just need to add the acquiring of queue_lock in 
nbd_clear_que to your previous patch and leave it at that. I'll code 
that up and test it.

Thanks,
Paul

Re: [NBD] Use per-device semaphore instead of BKL

[Herbert Xu]

On Sun, Nov 20, 2005 at 12:19:17PM -0500, Paul Clements wrote:
> 
> The dropping of the lock in nbd_do_it is actually critical to the way 
> nbd functions. nbd_do_it runs for the lifetime of the nbd device, so if 
> nbd_do_it were holding some lock (BKL or otherwise), we'd have big problems.

Why would you want to issue an ioctl from a different process while
nbd-client is still running?

Allow ioctl's while nbd_do_it is in progress is a *serious* bug.  For a
start, if someone else clears the socket then nbd_read_stat will crash.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NBD] Use per-device semaphore instead of BKL

[Paul Clements]

Herbert Xu wrote:
> On Sun, Nov 20, 2005 at 12:19:17PM -0500, Paul Clements wrote:
> 
>>The dropping of the lock in nbd_do_it is actually critical to the way 
>>nbd functions. nbd_do_it runs for the lifetime of the nbd device, so if 
>>nbd_do_it were holding some lock (BKL or otherwise), we'd have big problems.
> 
> 
> Why would you want to issue an ioctl from a different process while
> nbd-client is still running?

nbd-client -d (disconnect) is the main reason -- this functionality 
would be broken if we didn't allow ioctls anymore

> Allow ioctl's while nbd_do_it is in progress is a *serious* bug.  For a

Certain ioctls, yes. There's no harm in NBD_DISCONNECT, though.

> start, if someone else clears the socket then nbd_read_stat will crash.

I agree, NBD_CLEAR_SOCK from another process while nbd_do_it is running 
would cause problems. But, if the user-level tools are coded properly, 
this is not an issue.

Perhaps your ioctl lock scheme, with an exemption for NBD_DISCONNECT 
would work?

--
Paul

Re: [NBD] Use per-device semaphore instead of BKL

[Herbert Xu]

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

On Sun, Nov 20, 2005 at 04:42:23PM -0500, Paul Clements wrote:
> 
> nbd-client -d (disconnect) is the main reason -- this functionality 
> would be broken if we didn't allow ioctls anymore

Good point.  In fact my initial patch missed a pair of locks around
this case as well.  Here is a patch to move NBD_DISCONNECT outside
the ioctl_lock and add a tx_lock around it.


[NBD] Use per-device semaphore instead of BKL

Actually, we do have bigger problems :) The BKL is dropped every
time you sleep, and nbd_do_it is definitely a frequent sleeper :)

This isn't really an issue in practice though because the NBD
client program is single-threaded and doesn't share its file
descriptors with anyone else.

However, we shouldn't make it too easy for the user to shoot himself
in the foot.  If he's going to do that, let him at least pay for the
bullet :)

So here is a patch to use a per-device semaphore instead of the
BKL to protect the ioctl's against each other.

In order to preserve compatibility the ioctls NBD_DISCONNECT,
NBD_CLEAR_QUE and NBD_PRINT_DEBUG may be invoked without taking
the semaphore.

This patch also adds back the tx_lock for NBD_DISCONNECT that
went AWOL after the previous patch moved tx_lock outside of
nbd_send_req.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 4678 bytes --]

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -275,7 +275,7 @@ static int nbd_send_req(struct nbd_devic
 	return 0;
 
 error_out:
-	return 1;
+	return result;
 }
 
 static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
@@ -500,9 +500,9 @@ error_out:
 	return;
 }
 
-static int nbd_ioctl(struct inode *inode, struct file *file,
-		     unsigned int cmd, unsigned long arg)
+static long nbd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
+	struct inode *inode = file->f_dentry->d_inode;
 	struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
 	int error;
 	struct request sreq ;
@@ -528,13 +528,39 @@ static int nbd_ioctl(struct inode *inode
 		 */
 		sreq.sector = 0;
 		sreq.nr_sectors = 0;
-                if (!lo->sock)
-			return -EINVAL;
-                nbd_send_req(lo, &sreq);
-                return 0;
+
+		error = down_interruptible(&lo->tx_lock);
+		if (error)
+			return error;
+
+		error = -EINVAL;
+		if (lo->sock)
+			error = nbd_send_req(lo, &sreq);
+
+		up(&lo->tx_lock);
+		return error;
+
+	case NBD_CLEAR_QUE:
+		/*
+		 * This is for compatibility only.  The queue is always cleared
+		 * by NBD_DO_IT or NBD_CLEAR_SOCK.
+		 */
+		return 0;
+
+	case NBD_PRINT_DEBUG:
+		printk(KERN_INFO "%s: next = %p, prev = %p, head = %p\n",
+		       inode->i_bdev->bd_disk->disk_name,
+		       lo->queue_head.next, lo->queue_head.prev,
+		       &lo->queue_head);
+		return 0;
+	}
  
+	error = down_interruptible(&lo->ioctl_lock);
+	if (unlikely(error))
+		return error;
+
+	switch (cmd) {
 	case NBD_CLEAR_SOCK:
-		error = 0;
 		down(&lo->tx_lock);
 		lo->sock = NULL;
 		up(&lo->tx_lock);
@@ -544,10 +570,12 @@ static int nbd_ioctl(struct inode *inode
 		BUG_ON(!list_empty(&lo->queue_head));
 		if (file)
 			fput(file);
-		return error;
+		break;
+
 	case NBD_SET_SOCK:
+		error = -EBUSY;
 		if (lo->file)
-			return -EBUSY;
+			break;
 		error = -EINVAL;
 		file = fget(arg);
 		if (file) {
@@ -560,29 +588,34 @@ static int nbd_ioctl(struct inode *inode
 				fput(file);
 			}
 		}
-		return error;
+		break;
+
 	case NBD_SET_BLKSIZE:
 		lo->blksize = arg;
 		lo->bytesize &= ~(lo->blksize-1);
 		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_blocksize(inode->i_bdev, lo->blksize);
 		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		break;
+
 	case NBD_SET_SIZE:
 		lo->bytesize = arg & ~(lo->blksize-1);
 		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_blocksize(inode->i_bdev, lo->blksize);
 		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		break;
+
 	case NBD_SET_SIZE_BLOCKS:
 		lo->bytesize = ((u64) arg) * lo->blksize;
 		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_blocksize(inode->i_bdev, lo->blksize);
 		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		break;
+
 	case NBD_DO_IT:
+		error = -EINVAL;
 		if (!lo->file)
-			return -EINVAL;
+			break;
 		nbd_do_it(lo);
 		/* on return tidy up in case we have a signal */
 		/* Forcibly shutdown the socket causing all listeners
@@ -606,28 +639,21 @@ static int nbd_ioctl(struct inode *inode
 		printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
 		if (file)
 			fput(file);
-		return lo->harderror;
-	case NBD_CLEAR_QUE:
-		/*
-		 * This is for compatibility only.  The queue is always cleared
-		 * by NBD_DO_IT or NBD_CLEAR_SOCK.
-		 */
-		BUG_ON(!lo->sock && !list_empty(&lo->queue_head));
-		return 0;
-	case NBD_PRINT_DEBUG:
-		printk(KERN_INFO "%s: next = %p, prev = %p, head = %p\n",
-			inode->i_bdev->bd_disk->disk_name,
-			lo->queue_head.next, lo->queue_head.prev,
-			&lo->queue_head);
-		return 0;
+		error = lo->harderror;
+		break;
+
+	default:
+		error = -EINVAL;
+		break;
 	}
-	return -EINVAL;
+
+	up(&lo->ioctl_lock);
+	return error;
 }
 
-static struct block_device_operations nbd_fops =
-{
-	.owner =	THIS_MODULE,
-	.ioctl =	nbd_ioctl,
+static struct block_device_operations nbd_fops = {
+	.owner = THIS_MODULE,
+	.unlocked_ioctl = nbd_ioctl,
 };
 
 /*
@@ -685,6 +711,7 @@ static int __init nbd_init(void)
 		spin_lock_init(&nbd_dev[i].queue_lock);
 		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
 		init_MUTEX(&nbd_dev[i].tx_lock);
+		init_MUTEX(&nbd_dev[i].ioctl_lock);
 		init_waitqueue_head(&nbd_dev[i].active_wq);
 		nbd_dev[i].blksize = 1024;
 		nbd_dev[i].bytesize = 0x7ffffc00ULL << 10; /* 2TB */
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -58,6 +58,8 @@ struct nbd_device {
 	wait_queue_head_t active_wq;
 
 	struct semaphore tx_lock;
+	struct semaphore ioctl_lock;
+
 	struct gendisk *disk;
 	int blksize;
 	u64 bytesize;

Re: [NBD] Use per-device semaphore instead of BKL

[Paul Clements]

Herbert Xu wrote:
> On Sun, Nov 20, 2005 at 04:42:23PM -0500, Paul Clements wrote:
> 
>>nbd-client -d (disconnect) is the main reason -- this functionality 
>>would be broken if we didn't allow ioctls anymore
> 
> 
> Good point.  In fact my initial patch missed a pair of locks around
> this case as well.  Here is a patch to move NBD_DISCONNECT outside
> the ioctl_lock and add a tx_lock around it.

Yep, I think that will do it. I'll test these two patches some and let 
you know if I see any problems.

Thanks again.

--
Paul