[PULL 00/10] NBD updates for 4.3

[PULL 00/10] NBD updates for 4.3

[Markus Pargmann]

Hi Jens,

I hope this is not too late. Here are some NBD updates.

The most interesting one is probably the first patch, which improves the
timeout detection of NBD. The timout covers now the last reaction of the
server. If there are open requests and we don't receive anything from the
server within the timeout, the connection is killed.

The rest of the patches are smaller cleanups and some new debugfs entries about
the internal state of NBD.

Best Regards,

Markus


The following changes since commit d770e558e21961ad6cfdf0ff7df0eb5d7d4f0754:

  Linux 4.2-rc1 (2015-07-05 11:01:52 -0700)

are available in the git repository at:

  git://git.pengutronix.de/git/mpa/linux-nbd.git tags/nbd-for-4.3

for you to fetch changes up to 9bdcdf81aca98816fae86c724855e263454010f0:

  nbd: flags is a u32 variable (2015-08-17 08:08:50 +0200)

----------------------------------------------------------------
NBD updates for 4.3

----------------------------------------------------------------
Markus Pargmann (10):
      nbd: Fix timeout detection
      nbd: sock_shutdown, remove conditional lock
      nbd: restructure sock_shutdown
      nbd: Remove 'harderror' and propagate error properly
      nbd: Move clear queue debug message
      nbd: Remove variable 'pid'
      nbd: Add debugfs entries
      nbd: Change 'disconnect' to be boolean
      nbd: Rename functions for clearness of recv/send path
      nbd: flags is a u32 variable

 drivers/block/nbd.c | 362 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 290 insertions(+), 72 deletions(-)

-- 
2.4.6

[PATCH 01/10] nbd: Fix timeout detection

[Markus Pargmann]

At the moment the nbd timeout just detects hanging tcp operations. This
is not enough to detect a hanging or bad connection as expected of a
timeout.

This patch redesigns the timeout detection to include some more cases.
The timeout is now in relation to replies from the server. If the server
does not send replies within the timeout the connection will be shut
down.

The patch adds a continous timer 'timeout_timer' that is setup in one of
two cases:
 - The request list is empty and we are sending the first request out to
   the server. We want to have a reply within the given timeout,
   otherwise we consider the connection to be dead.
 - A server response was received. This means the server is still
   communicating with us. The timer is reset to the timeout value.

The timer is not stopped if the list becomes empty. It will just trigger
a timeout which will directly leave the handling routine again as the
request list is empty.

The whole patch does not use any additional explicit locking. The
list_empty() calls are safe to be used concurrently. The timer is locked
internally as we just use mod_timer and del_timer_sync().

The patch is based on the idea of Michal Belczyk with a previous
different implementation.

Cc: Michal Belczyk <belczyk@bsd.krakow.pl>
Cc: Hermann Lauer <Hermann.Lauer@iwr.uni-heidelberg.de>
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
Tested-by: Hermann Lauer <Hermann.Lauer@iwr.uni-heidelberg.de>
---
 drivers/block/nbd.c | 98 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 28 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0e385d8e9b86..7b9ae7a65c1e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -59,6 +59,10 @@ struct nbd_device {
 	pid_t pid; /* pid of nbd-client, if attached */
 	int xmit_timeout;
 	int disconnect; /* a disconnect has been requested by user */
+
+	struct timer_list timeout_timer;
+	struct task_struct *task_recv;
+	struct task_struct *task_send;
 };
 
 #define NBD_MAGIC 0x68797548
@@ -121,6 +125,7 @@ static void sock_shutdown(struct nbd_device *nbd, int lock)
 		dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
 		kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
 		nbd->sock = NULL;
+		del_timer_sync(&nbd->timeout_timer);
 	}
 	if (lock)
 		mutex_unlock(&nbd->tx_lock);
@@ -128,11 +133,23 @@ static void sock_shutdown(struct nbd_device *nbd, int lock)
 
 static void nbd_xmit_timeout(unsigned long arg)
 {
-	struct task_struct *task = (struct task_struct *)arg;
+	struct nbd_device *nbd = (struct nbd_device *)arg;
+	struct task_struct *task;
+
+	if (list_empty(&nbd->queue_head))
+		return;
+
+	nbd->disconnect = 1;
+
+	task = READ_ONCE(nbd->task_recv);
+	if (task)
+		force_sig(SIGKILL, task);
 
-	printk(KERN_WARNING "nbd: killing hung xmit (%s, pid: %d)\n",
-		task->comm, task->pid);
-	force_sig(SIGKILL, task);
+	task = READ_ONCE(nbd->task_send);
+	if (task)
+		force_sig(SIGKILL, nbd->task_send);
+
+	dev_err(nbd_to_dev(nbd), "Connection timed out, killed receiver and sender, shutting down connection\n");
 }
 
 /*
@@ -171,33 +188,12 @@ static int sock_xmit(struct nbd_device *nbd, int send, void *buf, int size,
 		msg.msg_controllen = 0;
 		msg.msg_flags = msg_flags | MSG_NOSIGNAL;
 
-		if (send) {
-			struct timer_list ti;
-
-			if (nbd->xmit_timeout) {
-				init_timer(&ti);
-				ti.function = nbd_xmit_timeout;
-				ti.data = (unsigned long)current;
-				ti.expires = jiffies + nbd->xmit_timeout;
-				add_timer(&ti);
-			}
+		if (send)
 			result = kernel_sendmsg(sock, &msg, &iov, 1, size);
-			if (nbd->xmit_timeout)
-				del_timer_sync(&ti);
-		} else
+		else
 			result = kernel_recvmsg(sock, &msg, &iov, 1, size,
 						msg.msg_flags);
 
-		if (signal_pending(current)) {
-			siginfo_t info;
-			printk(KERN_WARNING "nbd (pid %d: %s) got signal %d\n",
-				task_pid_nr(current), current->comm,
-				dequeue_signal_lock(current, &current->blocked, &info));
-			result = -EINTR;
-			sock_shutdown(nbd, !send);
-			break;
-		}
-
 		if (result <= 0) {
 			if (result == 0)
 				result = -EPIPE; /* short read */
@@ -210,6 +206,9 @@ static int sock_xmit(struct nbd_device *nbd, int send, void *buf, int size,
 	sigprocmask(SIG_SETMASK, &oldset, NULL);
 	tsk_restore_flags(current, pflags, PF_MEMALLOC);
 
+	if (!send && nbd->xmit_timeout)
+		mod_timer(&nbd->timeout_timer, jiffies + nbd->xmit_timeout);
+
 	return result;
 }
 
@@ -415,12 +414,26 @@ static int nbd_do_it(struct nbd_device *nbd)
 		return ret;
 	}
 
+	nbd->task_recv = current;
+
 	while ((req = nbd_read_stat(nbd)) != NULL)
 		nbd_end_request(nbd, req);
 
+	nbd->task_recv = NULL;
+
+	if (signal_pending(current)) {
+		siginfo_t info;
+
+		ret = dequeue_signal_lock(current, &current->blocked, &info);
+		dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal %d\n",
+			 task_pid_nr(current), current->comm, ret);
+		sock_shutdown(nbd, 1);
+		ret = -ETIMEDOUT;
+	}
+
 	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 	nbd->pid = 0;
-	return 0;
+	return ret;
 }
 
 static void nbd_clear_que(struct nbd_device *nbd)
@@ -482,6 +495,9 @@ static void nbd_handle_req(struct nbd_device *nbd, struct request *req)
 
 	nbd->active_req = req;
 
+	if (nbd->xmit_timeout && list_empty_careful(&nbd->queue_head))
+		mod_timer(&nbd->timeout_timer, jiffies + nbd->xmit_timeout);
+
 	if (nbd_send_req(nbd, req) != 0) {
 		dev_err(disk_to_dev(nbd->disk), "Request send failed\n");
 		req->errors++;
@@ -508,6 +524,8 @@ static int nbd_thread(void *data)
 	struct nbd_device *nbd = data;
 	struct request *req;
 
+	nbd->task_send = current;
+
 	set_user_nice(current, MIN_NICE);
 	while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {
 		/* wait for something to do */
@@ -515,6 +533,18 @@ static int nbd_thread(void *data)
 					 kthread_should_stop() ||
 					 !list_empty(&nbd->waiting_queue));
 
+		if (signal_pending(current)) {
+			siginfo_t info;
+			int ret;
+
+			ret = dequeue_signal_lock(current, &current->blocked,
+						  &info);
+			dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal %d\n",
+				 task_pid_nr(current), current->comm, ret);
+			sock_shutdown(nbd, 1);
+			break;
+		}
+
 		/* extract request */
 		if (list_empty(&nbd->waiting_queue))
 			continue;
@@ -528,6 +558,9 @@ static int nbd_thread(void *data)
 		/* handle request */
 		nbd_handle_req(nbd, req);
 	}
+
+	nbd->task_send = NULL;
+
 	return 0;
 }
 
@@ -648,6 +681,12 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 
 	case NBD_SET_TIMEOUT:
 		nbd->xmit_timeout = arg * HZ;
+		if (arg)
+			mod_timer(&nbd->timeout_timer,
+				  jiffies + nbd->xmit_timeout);
+		else
+			del_timer_sync(&nbd->timeout_timer);
+
 		return 0;
 
 	case NBD_SET_FLAGS:
@@ -842,6 +881,9 @@ static int __init nbd_init(void)
 		spin_lock_init(&nbd_dev[i].queue_lock);
 		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
 		mutex_init(&nbd_dev[i].tx_lock);
+		init_timer(&nbd_dev[i].timeout_timer);
+		nbd_dev[i].timeout_timer.function = nbd_xmit_timeout;
+		nbd_dev[i].timeout_timer.data = (unsigned long)&nbd_dev[i];
 		init_waitqueue_head(&nbd_dev[i].active_wq);
 		init_waitqueue_head(&nbd_dev[i].waiting_wq);
 		nbd_dev[i].blksize = 1024;
-- 
2.4.6

[PATCH 02/10] nbd: sock_shutdown, remove conditional lock

[Markus Pargmann]

Move the conditional lock from sock_shutdown into the surrounding code.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/block/nbd.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 7b9ae7a65c1e..ff59093c5dc1 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -117,18 +117,14 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
 /*
  * Forcibly shutdown the socket causing all listeners to error
  */
-static void sock_shutdown(struct nbd_device *nbd, int lock)
+static void sock_shutdown(struct nbd_device *nbd)
 {
-	if (lock)
-		mutex_lock(&nbd->tx_lock);
 	if (nbd->sock) {
 		dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
 		kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
 		nbd->sock = NULL;
 		del_timer_sync(&nbd->timeout_timer);
 	}
-	if (lock)
-		mutex_unlock(&nbd->tx_lock);
 }
 
 static void nbd_xmit_timeout(unsigned long arg)
@@ -427,7 +423,9 @@ static int nbd_do_it(struct nbd_device *nbd)
 		ret = dequeue_signal_lock(current, &current->blocked, &info);
 		dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal %d\n",
 			 task_pid_nr(current), current->comm, ret);
-		sock_shutdown(nbd, 1);
+		mutex_lock(&nbd->tx_lock);
+		sock_shutdown(nbd);
+		mutex_unlock(&nbd->tx_lock);
 		ret = -ETIMEDOUT;
 	}
 
@@ -541,7 +539,9 @@ static int nbd_thread(void *data)
 						  &info);
 			dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal %d\n",
 				 task_pid_nr(current), current->comm, ret);
-			sock_shutdown(nbd, 1);
+			mutex_lock(&nbd->tx_lock);
+			sock_shutdown(nbd);
+			mutex_unlock(&nbd->tx_lock);
 			break;
 		}
 
@@ -735,7 +735,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		mutex_lock(&nbd->tx_lock);
 		if (error)
 			return error;
-		sock_shutdown(nbd, 0);
+		sock_shutdown(nbd);
 		sock = nbd->sock;
 		nbd->sock = NULL;
 		nbd_clear_que(nbd);
-- 
2.4.6

[PATCH 03/10] nbd: restructure sock_shutdown

[Markus Pargmann]

This patch restructures sock_shutdown to avoid having the main code path
in an if block.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/block/nbd.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ff59093c5dc1..2c3661e4d364 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -119,12 +119,13 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
  */
 static void sock_shutdown(struct nbd_device *nbd)
 {
-	if (nbd->sock) {
-		dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
-		kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
-		nbd->sock = NULL;
-		del_timer_sync(&nbd->timeout_timer);
-	}
+	if (!nbd->sock)
+		return;
+
+	dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
+	kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
+	nbd->sock = NULL;
+	del_timer_sync(&nbd->timeout_timer);
 }
 
 static void nbd_xmit_timeout(unsigned long arg)
-- 
2.4.6

[PATCH 04/10] nbd: Remove 'harderror' and propagate error properly

[Markus Pargmann]

Instead of a variable 'harderror' we can simply try to correctly
propagate errors to the userspace.

This patch removes the harderror variable and passes errors through
error pointers and nbd_do_it back to the userspace.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/block/nbd.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 2c3661e4d364..8bce05d0df5e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -41,7 +41,6 @@
 
 struct nbd_device {
 	int flags;
-	int harderror;		/* Code of hard error			*/
 	struct socket * sock;	/* If == NULL, device is not ready, yet	*/
 	int magic;
 
@@ -329,26 +328,24 @@ static struct request *nbd_read_stat(struct nbd_device *nbd)
 	if (result <= 0) {
 		dev_err(disk_to_dev(nbd->disk),
 			"Receive control failed (result %d)\n", result);
-		goto harderror;
+		return ERR_PTR(result);
 	}
 
 	if (ntohl(reply.magic) != NBD_REPLY_MAGIC) {
 		dev_err(disk_to_dev(nbd->disk), "Wrong magic (0x%lx)\n",
 				(unsigned long)ntohl(reply.magic));
-		result = -EPROTO;
-		goto harderror;
+		return ERR_PTR(-EPROTO);
 	}
 
 	req = nbd_find_request(nbd, *(struct request **)reply.handle);
 	if (IS_ERR(req)) {
 		result = PTR_ERR(req);
 		if (result != -ENOENT)
-			goto harderror;
+			return ERR_PTR(result);
 
 		dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%p)\n",
 			reply.handle);
-		result = -EBADR;
-		goto harderror;
+		return ERR_PTR(-EBADR);
 	}
 
 	if (ntohl(reply.error)) {
@@ -376,9 +373,6 @@ static struct request *nbd_read_stat(struct nbd_device *nbd)
 		}
 	}
 	return req;
-harderror:
-	nbd->harderror = result;
-	return NULL;
 }
 
 static ssize_t pid_show(struct device *dev,
@@ -413,8 +407,15 @@ static int nbd_do_it(struct nbd_device *nbd)
 
 	nbd->task_recv = current;
 
-	while ((req = nbd_read_stat(nbd)) != NULL)
+	while (1) {
+		req = nbd_read_stat(nbd);
+		if (IS_ERR(req)) {
+			ret = PTR_ERR(req);
+			break;
+		}
+
 		nbd_end_request(nbd, req);
+	}
 
 	nbd->task_recv = NULL;
 
@@ -734,8 +735,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		kthread_stop(thread);
 
 		mutex_lock(&nbd->tx_lock);
-		if (error)
-			return error;
+
 		sock_shutdown(nbd);
 		sock = nbd->sock;
 		nbd->sock = NULL;
@@ -754,7 +754,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 			blkdev_reread_part(bdev);
 		if (nbd->disconnect) /* user requested, ignore socket errors */
 			return 0;
-		return nbd->harderror;
+		return error;
 	}
 
 	case NBD_CLEAR_QUE:
-- 
2.4.6

[PATCH 05/10] nbd: Move clear queue debug message

[Markus Pargmann]

This message was a warning without a reason. This patch moves it into
nbd_clear_que and transforms it to a debug message.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 8bce05d0df5e..5601523a8336 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -468,6 +468,7 @@ static void nbd_clear_que(struct nbd_device *nbd)
 		req->errors++;
 		nbd_end_request(nbd, req);
 	}
+	dev_dbg(disk_to_dev(nbd->disk), "queue cleared\n");
 }
 
 
@@ -740,7 +741,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		sock = nbd->sock;
 		nbd->sock = NULL;
 		nbd_clear_que(nbd);
-		dev_warn(disk_to_dev(nbd->disk), "queue cleared\n");
 		kill_bdev(bdev);
 		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
 		set_device_ro(bdev, false);
-- 
2.4.6

[PATCH 06/10] nbd: Remove variable 'pid'

[Markus Pargmann]

This patch uses nbd->task_recv to determine the value of the previously
used variable 'pid' for sysfs.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5601523a8336..0075554a05b7 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -55,7 +55,6 @@ struct nbd_device {
 	struct gendisk *disk;
 	int blksize;
 	loff_t bytesize;
-	pid_t pid; /* pid of nbd-client, if attached */
 	int xmit_timeout;
 	int disconnect; /* a disconnect has been requested by user */
 
@@ -379,9 +378,9 @@ static ssize_t pid_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
 	struct gendisk *disk = dev_to_disk(dev);
+	struct nbd_device *nbd = (struct nbd_device *)disk->private_data;
 
-	return sprintf(buf, "%ld\n",
-		(long) ((struct nbd_device *)disk->private_data)->pid);
+	return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv));
 }
 
 static struct device_attribute pid_attr = {
@@ -397,16 +396,16 @@ static int nbd_do_it(struct nbd_device *nbd)
 	BUG_ON(nbd->magic != NBD_MAGIC);
 
 	sk_set_memalloc(nbd->sock->sk);
-	nbd->pid = task_pid_nr(current);
+
+	nbd->task_recv = current;
+
 	ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
 	if (ret) {
 		dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
-		nbd->pid = 0;
+		nbd->task_recv = NULL;
 		return ret;
 	}
 
-	nbd->task_recv = current;
-
 	while (1) {
 		req = nbd_read_stat(nbd);
 		if (IS_ERR(req)) {
@@ -417,6 +416,8 @@ static int nbd_do_it(struct nbd_device *nbd)
 		nbd_end_request(nbd, req);
 	}
 
+	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
+
 	nbd->task_recv = NULL;
 
 	if (signal_pending(current)) {
@@ -431,8 +432,6 @@ static int nbd_do_it(struct nbd_device *nbd)
 		ret = -ETIMEDOUT;
 	}
 
-	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
-	nbd->pid = 0;
 	return ret;
 }
 
@@ -708,7 +707,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		struct socket *sock;
 		int error;
 
-		if (nbd->pid)
+		if (nbd->task_recv)
 			return -EBUSY;
 		if (!nbd->sock)
 			return -EINVAL;
-- 
2.4.6

[PATCH 07/10] nbd: Add debugfs entries

[Markus Pargmann]

Add some debugfs files that help to understand the internal state of
NBD. This exports the different sizes, flags, tasks and so on.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 177 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0075554a05b7..188fe0d2b1ac 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -33,6 +33,7 @@
 #include <linux/net.h>
 #include <linux/kthread.h>
 #include <linux/types.h>
+#include <linux/debugfs.h>
 
 #include <asm/uaccess.h>
 #include <asm/types.h>
@@ -61,8 +62,18 @@ struct nbd_device {
 	struct timer_list timeout_timer;
 	struct task_struct *task_recv;
 	struct task_struct *task_send;
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	struct dentry *dbg_dir;
+#endif
 };
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static struct dentry *nbd_dbg_dir;
+#endif
+
+#define nbd_name(nbd) ((nbd)->disk->disk_name)
+
 #define NBD_MAGIC 0x68797548
 
 static unsigned int nbds_max = 16;
@@ -609,6 +620,9 @@ static void do_nbd_request(struct request_queue *q)
 	}
 }
 
+static int nbd_dev_dbg_init(struct nbd_device *nbd);
+static void nbd_dev_dbg_close(struct nbd_device *nbd);
+
 /* Must be called with tx_lock held */
 
 static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
@@ -725,13 +739,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 			blk_queue_flush(nbd->disk->queue, 0);
 
 		thread = kthread_run(nbd_thread, nbd, "%s",
-				     nbd->disk->disk_name);
+				     nbd_name(nbd));
 		if (IS_ERR(thread)) {
 			mutex_lock(&nbd->tx_lock);
 			return PTR_ERR(thread);
 		}
 
+		nbd_dev_dbg_init(nbd);
 		error = nbd_do_it(nbd);
+		nbd_dev_dbg_close(nbd);
 		kthread_stop(thread);
 
 		mutex_lock(&nbd->tx_lock);
@@ -797,6 +813,161 @@ static const struct block_device_operations nbd_fops =
 	.ioctl =	nbd_ioctl,
 };
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+
+static int nbd_dbg_tasks_show(struct seq_file *s, void *unused)
+{
+	struct nbd_device *nbd = s->private;
+
+	if (nbd->task_recv)
+		seq_printf(s, "recv: %d\n", task_pid_nr(nbd->task_recv));
+	if (nbd->task_send)
+		seq_printf(s, "send: %d\n", task_pid_nr(nbd->task_send));
+
+	return 0;
+}
+
+static int nbd_dbg_tasks_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, nbd_dbg_tasks_show, inode->i_private);
+}
+
+static const struct file_operations nbd_dbg_tasks_ops = {
+	.open = nbd_dbg_tasks_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static int nbd_dbg_flags_show(struct seq_file *s, void *unused)
+{
+	struct nbd_device *nbd = s->private;
+	u32 flags = nbd->flags;
+
+	seq_printf(s, "Hex: 0x%08x\n\n", flags);
+
+	seq_puts(s, "Known flags:\n");
+
+	if (flags & NBD_FLAG_HAS_FLAGS)
+		seq_puts(s, "NBD_FLAG_HAS_FLAGS\n");
+	if (flags & NBD_FLAG_READ_ONLY)
+		seq_puts(s, "NBD_FLAG_READ_ONLY\n");
+	if (flags & NBD_FLAG_SEND_FLUSH)
+		seq_puts(s, "NBD_FLAG_SEND_FLUSH\n");
+	if (flags & NBD_FLAG_SEND_TRIM)
+		seq_puts(s, "NBD_FLAG_SEND_TRIM\n");
+
+	return 0;
+}
+
+static int nbd_dbg_flags_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, nbd_dbg_flags_show, inode->i_private);
+}
+
+static const struct file_operations nbd_dbg_flags_ops = {
+	.open = nbd_dbg_flags_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static int nbd_dev_dbg_init(struct nbd_device *nbd)
+{
+	struct dentry *dir;
+	struct dentry *f;
+
+	dir = debugfs_create_dir(nbd_name(nbd), nbd_dbg_dir);
+	if (IS_ERR_OR_NULL(dir)) {
+		dev_err(nbd_to_dev(nbd), "Failed to create debugfs dir for '%s' (%ld)\n",
+			nbd_name(nbd), PTR_ERR(dir));
+		return PTR_ERR(dir);
+	}
+	nbd->dbg_dir = dir;
+
+	f = debugfs_create_file("tasks", 0444, dir, nbd, &nbd_dbg_tasks_ops);
+	if (IS_ERR_OR_NULL(f)) {
+		dev_err(nbd_to_dev(nbd), "Failed to create debugfs file 'tasks', %ld\n",
+			PTR_ERR(f));
+		return PTR_ERR(f);
+	}
+
+	f = debugfs_create_u64("size_bytes", 0444, dir, &nbd->bytesize);
+	if (IS_ERR_OR_NULL(f)) {
+		dev_err(nbd_to_dev(nbd), "Failed to create debugfs file 'size_bytes', %ld\n",
+			PTR_ERR(f));
+		return PTR_ERR(f);
+	}
+
+	f = debugfs_create_u32("timeout", 0444, dir, &nbd->xmit_timeout);
+	if (IS_ERR_OR_NULL(f)) {
+		dev_err(nbd_to_dev(nbd), "Failed to create debugfs file 'timeout', %ld\n",
+			PTR_ERR(f));
+		return PTR_ERR(f);
+	}
+
+	f = debugfs_create_u32("blocksize", 0444, dir, &nbd->blksize);
+	if (IS_ERR_OR_NULL(f)) {
+		dev_err(nbd_to_dev(nbd), "Failed to create debugfs file 'blocksize', %ld\n",
+			PTR_ERR(f));
+		return PTR_ERR(f);
+	}
+
+	f = debugfs_create_file("flags", 0444, dir, &nbd, &nbd_dbg_flags_ops);
+	if (IS_ERR_OR_NULL(f)) {
+		dev_err(nbd_to_dev(nbd), "Failed to create debugfs file 'flags', %ld\n",
+			PTR_ERR(f));
+		return PTR_ERR(f);
+	}
+
+	return 0;
+}
+
+static void nbd_dev_dbg_close(struct nbd_device *nbd)
+{
+	debugfs_remove_recursive(nbd->dbg_dir);
+}
+
+static int nbd_dbg_init(void)
+{
+	struct dentry *dbg_dir;
+
+	dbg_dir = debugfs_create_dir("nbd", NULL);
+	if (IS_ERR(dbg_dir))
+		return PTR_ERR(dbg_dir);
+
+	nbd_dbg_dir = dbg_dir;
+
+	return 0;
+}
+
+static void nbd_dbg_close(void)
+{
+	debugfs_remove_recursive(nbd_dbg_dir);
+}
+
+#else  /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
+static int nbd_dev_dbg_init(struct nbd_device *nbd)
+{
+	return 0;
+}
+
+static void nbd_dev_dbg_close(struct nbd_device *nbd)
+{
+}
+
+static int nbd_dbg_init(void)
+{
+	return 0;
+}
+
+static void nbd_dbg_close(void)
+{
+}
+
+#endif
+
 /*
  * And here should be modules and kernel interface 
  *  (Just smiley confuses emacs :-)
@@ -874,6 +1045,8 @@ static int __init nbd_init(void)
 
 	printk(KERN_INFO "nbd: registered device at major %d\n", NBD_MAJOR);
 
+	nbd_dbg_init();
+
 	for (i = 0; i < nbds_max; i++) {
 		struct gendisk *disk = nbd_dev[i].disk;
 		nbd_dev[i].magic = NBD_MAGIC;
@@ -910,6 +1083,9 @@ out:
 static void __exit nbd_cleanup(void)
 {
 	int i;
+
+	nbd_dbg_close();
+
 	for (i = 0; i < nbds_max; i++) {
 		struct gendisk *disk = nbd_dev[i].disk;
 		nbd_dev[i].magic = 0;
-- 
2.4.6

[PATCH 08/10] nbd: Change 'disconnect' to be boolean

[Markus Pargmann]

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
Acked-by: Pavel Machek  <pavel@ucw.cz>
---
 drivers/block/nbd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 188fe0d2b1ac..44160a9e493e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -57,7 +57,7 @@ struct nbd_device {
 	int blksize;
 	loff_t bytesize;
 	int xmit_timeout;
-	int disconnect; /* a disconnect has been requested by user */
+	bool disconnect; /* a disconnect has been requested by user */
 
 	struct timer_list timeout_timer;
 	struct task_struct *task_recv;
@@ -145,7 +145,7 @@ static void nbd_xmit_timeout(unsigned long arg)
 	if (list_empty(&nbd->queue_head))
 		return;
 
-	nbd->disconnect = 1;
+	nbd->disconnect = true;
 
 	task = READ_ONCE(nbd->task_recv);
 	if (task)
@@ -646,7 +646,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		if (!nbd->sock)
 			return -EINVAL;
 
-		nbd->disconnect = 1;
+		nbd->disconnect = true;
 
 		nbd_send_req(nbd, &sreq);
 		return 0;
@@ -674,7 +674,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 			nbd->sock = sock;
 			if (max_part > 0)
 				bdev->bd_invalidated = 1;
-			nbd->disconnect = 0; /* we're connected now */
+			nbd->disconnect = false; /* we're connected now */
 			return 0;
 		}
 		return -EINVAL;
-- 
2.4.6

[PATCH 09/10] nbd: Rename functions for clearness of recv/send path

[Markus Pargmann]

This patch renames functions so that it is clear what the function does.
Otherwise it is not directly understandable what for example 'do_it' means.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 44160a9e493e..9862c3e64ff2 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -399,7 +399,7 @@ static struct device_attribute pid_attr = {
 	.show = pid_show,
 };
 
-static int nbd_do_it(struct nbd_device *nbd)
+static int nbd_thread_recv(struct nbd_device *nbd)
 {
 	struct request *req;
 	int ret;
@@ -530,7 +530,7 @@ error_out:
 	nbd_end_request(nbd, req);
 }
 
-static int nbd_thread(void *data)
+static int nbd_thread_send(void *data)
 {
 	struct nbd_device *nbd = data;
 	struct request *req;
@@ -584,7 +584,7 @@ static int nbd_thread(void *data)
  *   { printk( "Warning: Ignoring result!\n"); nbd_end_request( req ); }
  */
 
-static void do_nbd_request(struct request_queue *q)
+static void nbd_request_handler(struct request_queue *q)
 		__releases(q->queue_lock) __acquires(q->queue_lock)
 {
 	struct request *req;
@@ -738,7 +738,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		else
 			blk_queue_flush(nbd->disk->queue, 0);
 
-		thread = kthread_run(nbd_thread, nbd, "%s",
+		thread = kthread_run(nbd_thread_send, nbd, "%s",
 				     nbd_name(nbd));
 		if (IS_ERR(thread)) {
 			mutex_lock(&nbd->tx_lock);
@@ -746,7 +746,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		}
 
 		nbd_dev_dbg_init(nbd);
-		error = nbd_do_it(nbd);
+		error = nbd_thread_recv(nbd);
 		nbd_dev_dbg_close(nbd);
 		kthread_stop(thread);
 
@@ -1021,7 +1021,7 @@ static int __init nbd_init(void)
 		 * every gendisk to have its very own request_queue struct.
 		 * These structs are big so we dynamically allocate them.
 		 */
-		disk->queue = blk_init_queue(do_nbd_request, &nbd_lock);
+		disk->queue = blk_init_queue(nbd_request_handler, &nbd_lock);
 		if (!disk->queue) {
 			put_disk(disk);
 			goto out;
-- 
2.4.6

[PATCH 10/10] nbd: flags is a u32 variable

[Markus Pargmann]

The flags variable is used as u32 variable. This patch changes the type
to be u32.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9862c3e64ff2..1176a3b27a7e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -41,7 +41,7 @@
 #include <linux/nbd.h>
 
 struct nbd_device {
-	int flags;
+	u32 flags;
 	struct socket * sock;	/* If == NULL, device is not ready, yet	*/
 	int magic;
 
-- 
2.4.6

Re: [PULL 00/10] NBD updates for 4.3

[Jens Axboe]

On 08/17/2015 12:19 AM, Markus Pargmann wrote:
> Hi Jens,
>
> I hope this is not too late. Here are some NBD updates.
>
> The most interesting one is probably the first patch, which improves the
> timeout detection of NBD. The timout covers now the last reaction of the
> server. If there are open requests and we don't receive anything from the
> server within the timeout, the connection is killed.
>
> The rest of the patches are smaller cleanups and some new debugfs entries about
> the internal state of NBD.

Applied for 4.3, thanks.

-- 
Jens Axboe

Re: [PATCH 01/10] nbd: Fix timeout detection

[Ben Hutchings]

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

On Mon, 2015-08-17 at 08:20 +0200, Markus Pargmann wrote:
> At the moment the nbd timeout just detects hanging tcp operations. This
> is not enough to detect a hanging or bad connection as expected of a
> timeout.
> 
> This patch redesigns the timeout detection to include some more cases.
> The timeout is now in relation to replies from the server. If the server
> does not send replies within the timeout the connection will be shut
> down.
> 
> The patch adds a continous timer 'timeout_timer' that is setup in one of
> two cases:
>  - The request list is empty and we are sending the first request out to
>    the server. We want to have a reply within the given timeout,
>    otherwise we consider the connection to be dead.
>  - A server response was received. This means the server is still
>    communicating with us. The timer is reset to the timeout value.
> 
> The timer is not stopped if the list becomes empty. It will just trigger
> a timeout which will directly leave the handling routine again as the
> request list is empty.
> 
> The whole patch does not use any additional explicit locking. The
> list_empty() calls are safe to be used concurrently. The timer is locked
> internally as we just use mod_timer and del_timer_sync().

This is crazy.  The timer is locked internally but the tasks are not.
So it is possible for the timeout handler to kill a task after it
exited from nbd_do_it()/nbd_thread_recv(), or after it exited entirely
(use-after-free).

[...]
> +> 	> task = READ_ONCE(nbd->task_send);
> +> 	> if (task)
> +> 	> 	> force_sig(SIGKILL, nbd->task_send);
[...]

And this is just... what?  What is the point of using READ_ONCE() if
you're going to look up nbd->task_send again?

Ben.

-- 
Ben Hutchings
All extremists should be taken out and shot.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 01/10] nbd: Fix timeout detection

[Markus Pargmann]

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

Hi,

On Mon, Sep 28, 2015 at 01:27:44AM +0100, Ben Hutchings wrote:
> On Mon, 2015-08-17 at 08:20 +0200, Markus Pargmann wrote:
> > At the moment the nbd timeout just detects hanging tcp operations. This
> > is not enough to detect a hanging or bad connection as expected of a
> > timeout.
> > 
> > This patch redesigns the timeout detection to include some more cases.
> > The timeout is now in relation to replies from the server. If the server
> > does not send replies within the timeout the connection will be shut
> > down.
> > 
> > The patch adds a continous timer 'timeout_timer' that is setup in one of
> > two cases:
> >  - The request list is empty and we are sending the first request out to
> >    the server. We want to have a reply within the given timeout,
> >    otherwise we consider the connection to be dead.
> >  - A server response was received. This means the server is still
> >    communicating with us. The timer is reset to the timeout value.
> > 
> > The timer is not stopped if the list becomes empty. It will just trigger
> > a timeout which will directly leave the handling routine again as the
> > request list is empty.
> > 
> > The whole patch does not use any additional explicit locking. The
> > list_empty() calls are safe to be used concurrently. The timer is locked
> > internally as we just use mod_timer and del_timer_sync().
> 
> This is crazy.  The timer is locked internally but the tasks are not.
> So it is possible for the timeout handler to kill a task after it
> exited from nbd_do_it()/nbd_thread_recv(), or after it exited entirely
> (use-after-free).

Indeed, thanks. I am working on a patch to fix this issue.

Best Regards,

Markus

> 
> [...]
> > +> 	> task = READ_ONCE(nbd->task_send);
> > +> 	> if (task)
> > +> 	> 	> force_sig(SIGKILL, nbd->task_send);
> [...]
> 
> And this is just... what?  What is the point of using READ_ONCE() if
> you're going to look up nbd->task_send again?
> 
> Ben.
> 
> -- 
> Ben Hutchings
> All extremists should be taken out and shot.



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] nbd: Add locking for tasks

[Markus Pargmann]

The timeout handling introduced in
	7e2893a16d3e (nbd: Fix timeout detection)
introduces a race condition which may lead to killing of tasks that are
not in nbd context anymore. This was not observed or reproducable yet.

This patch adds locking to critical use of task_recv and task_send to
avoid killing tasks that already left the NBD thread functions. This
lock is only acquired if a timeout occures or the nbd device
starts/stops.

Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 039c7c4f0539..1a70852ac808 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -60,6 +60,7 @@ struct nbd_device {
 	bool disconnect; /* a disconnect has been requested by user */
 
 	struct timer_list timeout_timer;
+	spinlock_t tasks_lock;
 	struct task_struct *task_recv;
 	struct task_struct *task_send;
 
@@ -140,21 +141,23 @@ static void sock_shutdown(struct nbd_device *nbd)
 static void nbd_xmit_timeout(unsigned long arg)
 {
 	struct nbd_device *nbd = (struct nbd_device *)arg;
-	struct task_struct *task;
+	unsigned long flags;
 
 	if (list_empty(&nbd->queue_head))
 		return;
 
 	nbd->disconnect = true;
 
-	task = READ_ONCE(nbd->task_recv);
-	if (task)
-		force_sig(SIGKILL, task);
+	spin_lock_irqsave(&nbd->tasks_lock, flags);
+
+	if (nbd->task_recv)
+		force_sig(SIGKILL, nbd->task_recv);
 
-	task = READ_ONCE(nbd->task_send);
-	if (task)
+	if (nbd->task_send)
 		force_sig(SIGKILL, nbd->task_send);
 
+	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+
 	dev_err(nbd_to_dev(nbd), "Connection timed out, killed receiver and sender, shutting down connection\n");
 }
 
@@ -403,17 +406,24 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 {
 	struct request *req;
 	int ret;
+	unsigned long flags;
 
 	BUG_ON(nbd->magic != NBD_MAGIC);
 
 	sk_set_memalloc(nbd->sock->sk);
 
+	spin_lock_irqsave(&nbd->tasks_lock, flags);
 	nbd->task_recv = current;
+	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
 
 	ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
 	if (ret) {
 		dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
+
+		spin_lock_irqsave(&nbd->tasks_lock, flags);
 		nbd->task_recv = NULL;
+		spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+
 		return ret;
 	}
 
@@ -429,7 +439,9 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 
 	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 
+	spin_lock_irqsave(&nbd->tasks_lock, flags);
 	nbd->task_recv = NULL;
+	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
 
 	if (signal_pending(current)) {
 		siginfo_t info;
@@ -534,8 +546,11 @@ static int nbd_thread_send(void *data)
 {
 	struct nbd_device *nbd = data;
 	struct request *req;
+	unsigned long flags;
 
+	spin_lock_irqsave(&nbd->tasks_lock, flags);
 	nbd->task_send = current;
+	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
 
 	set_user_nice(current, MIN_NICE);
 	while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {
@@ -572,7 +587,15 @@ static int nbd_thread_send(void *data)
 		nbd_handle_req(nbd, req);
 	}
 
+	spin_lock_irqsave(&nbd->tasks_lock, flags);
 	nbd->task_send = NULL;
+	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+
+	/* Clear maybe pending signals */
+	if (signal_pending(current)) {
+		siginfo_t info;
+		dequeue_signal_lock(current, &current->blocked, &info);
+	}
 
 	return 0;
 }
@@ -1027,6 +1050,7 @@ static int __init nbd_init(void)
 		nbd_dev[i].magic = NBD_MAGIC;
 		INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
 		spin_lock_init(&nbd_dev[i].queue_lock);
+		spin_lock_init(&nbd_dev[i].tasks_lock);
 		INIT_LIST_HEAD(&nbd_dev[i].queue_head);
 		mutex_init(&nbd_dev[i].tx_lock);
 		init_timer(&nbd_dev[i].timeout_timer);
-- 
2.6.0

Re: [PATCH] nbd: Add locking for tasks

[Ben Hutchings]

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

On Tue, 2015-10-06 at 20:03 +0200, Markus Pargmann wrote:
> The timeout handling introduced in
> 	> 7e2893a16d3e (nbd: Fix timeout detection)
> introduces a race condition which may lead to killing of tasks that are
> not in nbd context anymore. This was not observed or reproducable yet.
> 
> This patch adds locking to critical use of task_recv and task_send to
> avoid killing tasks that already left the NBD thread functions. This
> lock is only acquired if a timeout occures or the nbd device
> starts/stops.
> 
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Reviewed-by: Ben Hutchings <ben@decadent.org.uk>

You could add 'Fixes: 7e2893a16d3e ("nbd: Fix timeout detection")' to
the commit message as well.

nbd_dbg_tasks_show() can still race with thread exit and two tasks can 
race to become the receive thread, but those aren't new bugs.

Ben.

> ---
>  drivers/block/nbd.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 039c7c4f0539..1a70852ac808 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -60,6 +60,7 @@ struct nbd_device {
>  > 	> bool disconnect; /* a disconnect has been requested by user */
>  
>  > 	> struct timer_list timeout_timer;
> +> 	> spinlock_t tasks_lock;
>  > 	> struct task_struct *task_recv;
>  > 	> struct task_struct *task_send;
>  
> @@ -140,21 +141,23 @@ static void sock_shutdown(struct nbd_device *nbd)
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
>  > 	> struct nbd_device *nbd = (struct nbd_device *)arg;
> -> 	> struct task_struct *task;
> +> 	> unsigned long flags;
>  
>  > 	> if (list_empty(&nbd->queue_head))
>  > 	> 	> return;
>  
>  > 	> nbd->disconnect = true;
>  
> -> 	> task = READ_ONCE(nbd->task_recv);
> -> 	> if (task)
> -> 	> 	> force_sig(SIGKILL, task);
> +> 	> spin_lock_irqsave(&nbd->tasks_lock, flags);
> +
> +> 	> if (nbd->task_recv)
> +> 	> 	> force_sig(SIGKILL, nbd->task_recv);
>  
> -> 	> task = READ_ONCE(nbd->task_send);
> -> 	> if (task)
> +> 	> if (nbd->task_send)
>  > 	> 	> force_sig(SIGKILL, nbd->task_send);
>  
> +> 	> spin_unlock_irqrestore(&nbd->tasks_lock, flags);
> +
>  > 	> dev_err(nbd_to_dev(nbd), "Connection timed out, killed receiver and sender, shutting down connection\n");
>  }
>  
> @@ -403,17 +406,24 @@ static int nbd_thread_recv(struct nbd_device *nbd)
>  {
>  > 	> struct request *req;
>  > 	> int ret;
> +> 	> unsigned long flags;
>  
>  > 	> BUG_ON(nbd->magic != NBD_MAGIC);
>  
>  > 	> sk_set_memalloc(nbd->sock->sk);
>  
> +> 	> spin_lock_irqsave(&nbd->tasks_lock, flags);
>  > 	> nbd->task_recv = current;
> +> 	> spin_unlock_irqrestore(&nbd->tasks_lock, flags);
>  
>  > 	> ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
>  > 	> if (ret) {
>  > 	> 	> dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
> +
> +> 	> 	> spin_lock_irqsave(&nbd->tasks_lock, flags);
>  > 	> 	> nbd->task_recv = NULL;
> +> 	> 	> spin_unlock_irqrestore(&nbd->tasks_lock, flags);
> +
>  > 	> 	> return ret;
>  > 	> }
>  
> @@ -429,7 +439,9 @@ static int nbd_thread_recv(struct nbd_device *nbd)
>  
>  > 	> device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
>  
> +> 	> spin_lock_irqsave(&nbd->tasks_lock, flags);
>  > 	> nbd->task_recv = NULL;
> +> 	> spin_unlock_irqrestore(&nbd->tasks_lock, flags);
>  
>  > 	> if (signal_pending(current)) {
>  > 	> 	> siginfo_t info;
> @@ -534,8 +546,11 @@ static int nbd_thread_send(void *data)
>  {
>  > 	> struct nbd_device *nbd = data;
>  > 	> struct request *req;
> +> 	> unsigned long flags;
>  
> +> 	> spin_lock_irqsave(&nbd->tasks_lock, flags);
>  > 	> nbd->task_send = current;
> +> 	> spin_unlock_irqrestore(&nbd->tasks_lock, flags);
>  
>  > 	> set_user_nice(current, MIN_NICE);
>  > 	> while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {
> @@ -572,7 +587,15 @@ static int nbd_thread_send(void *data)
>  > 	> 	> nbd_handle_req(nbd, req);
>  > 	> }
>  
> +> 	> spin_lock_irqsave(&nbd->tasks_lock, flags);
>  > 	> nbd->task_send = NULL;
> +> 	> spin_unlock_irqrestore(&nbd->tasks_lock, flags);
> +
> +> 	> /* Clear maybe pending signals */
> +> 	> if (signal_pending(current)) {
> +> 	> 	> siginfo_t info;
> +> 	> 	> dequeue_signal_lock(current, ¤t->blocked, &info);
> +> 	> }
>  
>  > 	> return 0;
>  }
> @@ -1027,6 +1050,7 @@ static int __init nbd_init(void)
>  > 	> 	> nbd_dev[i].magic = NBD_MAGIC;
>  > 	> 	> INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
>  > 	> 	> spin_lock_init(&nbd_dev[i].queue_lock);
> +> 	> 	> spin_lock_init(&nbd_dev[i].tasks_lock);
>  > 	> 	> INIT_LIST_HEAD(&nbd_dev[i].queue_head);
>  > 	> 	> mutex_init(&nbd_dev[i].tx_lock);
>  > 	> 	> init_timer(&nbd_dev[i].timeout_timer);
-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] nbd: Add locking for tasks

[Jens Axboe]

On 10/08/2015 02:14 PM, Ben Hutchings wrote:
> On Tue, 2015-10-06 at 20:03 +0200, Markus Pargmann wrote:
>> The timeout handling introduced in
>> 	> 7e2893a16d3e (nbd: Fix timeout detection)
>> introduces a race condition which may lead to killing of tasks that are
>> not in nbd context anymore. This was not observed or reproducable yet.
>>
>> This patch adds locking to critical use of task_recv and task_send to
>> avoid killing tasks that already left the NBD thread functions. This
>> lock is only acquired if a timeout occures or the nbd device
>> starts/stops.
>>
>> Reported-by: Ben Hutchings <ben@decadent.org.uk>
>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>
> Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
>
> You could add 'Fixes: 7e2893a16d3e ("nbd: Fix timeout detection")' to
> the commit message as well.

I've added your review and the Fixes and added it for this series. 
Thanks guys.

-- 
Jens Axboe