[PATCH 0/4] nbd: Remove signal usage

[PATCH 0/4] nbd: Remove signal usage

[Markus Pargmann]

Hi,

this is a try to remove all the signals from NBD. The first patch replaces the
signals. The other patches are some cleanups I made on the way.

This should solve the kthread_run() problems as well.

Best Regards,

Markus


Markus Pargmann (4):
  nbd: Remove signal usage
  nbd: Timeouts are not user requested disconnects
  nbd: Cleanup reset of nbd and bdev after a disconnect
  nbd: Move flag parsing to a function

 drivers/block/nbd.c | 189 +++++++++++++++++++++++++---------------------------
 1 file changed, 89 insertions(+), 100 deletions(-)

-- 
2.6.1

[PATCH 1/4] nbd: Remove signal usage

[Markus Pargmann]

As discussed on the mailing list, the usage of signals for timeout
handling has a lot of potential issues. The nbd driver used for some
time signals for timeouts. These signals where able to get the threads
out of the blocking socket operations.

This patch removes all signal usage and uses a socket shutdown instead.
The socket descriptor itself is cleared later when the whole nbd device
is closed.

The tasks_lock is removed as we do not depend on this anymore. Instead
a new lock for the socket is introduced so we can safely work with the
socket in the timeout handler outside of the two main threads.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 119 +++++++++++++++++++---------------------------------
 1 file changed, 42 insertions(+), 77 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index b9e4179a950a..d14102d1e9ba 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -60,7 +60,8 @@ struct nbd_device {
 	bool disconnect; /* a disconnect has been requested by user */
 
 	struct timer_list timeout_timer;
-	spinlock_t tasks_lock;
+	/* protects initialization and shutdown of the socket */
+	spinlock_t sock_lock;
 	struct task_struct *task_recv;
 	struct task_struct *task_send;
 
@@ -129,13 +130,19 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
  */
 static void sock_shutdown(struct nbd_device *nbd)
 {
+	spin_lock_irq(&nbd->sock_lock);
+
 	if (!nbd->sock)
-		return;
+		goto out;
 
 	dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
 	kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
+	sockfd_put(nbd->sock);
 	nbd->sock = NULL;
 	del_timer_sync(&nbd->timeout_timer);
+
+out:
+	spin_unlock_irq(&nbd->sock_lock);
 }
 
 static void nbd_xmit_timeout(unsigned long arg)
@@ -148,17 +155,15 @@ static void nbd_xmit_timeout(unsigned long arg)
 
 	nbd->disconnect = true;
 
-	spin_lock_irqsave(&nbd->tasks_lock, flags);
+	spin_lock_irqsave(&nbd->sock_lock, flags);
 
-	if (nbd->task_recv)
-		force_sig(SIGKILL, nbd->task_recv);
 
-	if (nbd->task_send)
-		force_sig(SIGKILL, nbd->task_send);
+	if (nbd->sock)
+		kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
 
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irqrestore(&nbd->sock_lock, flags);
 
-	dev_err(nbd_to_dev(nbd), "Connection timed out, killed receiver and sender, shutting down connection\n");
+	dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
 }
 
 /*
@@ -171,7 +176,6 @@ static int sock_xmit(struct nbd_device *nbd, int send, void *buf, int size,
 	int result;
 	struct msghdr msg;
 	struct kvec iov;
-	sigset_t blocked, oldset;
 	unsigned long pflags = current->flags;
 
 	if (unlikely(!sock)) {
@@ -181,11 +185,6 @@ static int sock_xmit(struct nbd_device *nbd, int send, void *buf, int size,
 		return -EINVAL;
 	}
 
-	/* Allow interception of SIGKILL only
-	 * Don't allow other signals to interrupt the transmission */
-	siginitsetinv(&blocked, sigmask(SIGKILL));
-	sigprocmask(SIG_SETMASK, &blocked, &oldset);
-
 	current->flags |= PF_MEMALLOC;
 	do {
 		sock->sk->sk_allocation = GFP_NOIO | __GFP_MEMALLOC;
@@ -212,7 +211,6 @@ static int sock_xmit(struct nbd_device *nbd, int send, void *buf, int size,
 		buf += result;
 	} while (size > 0);
 
-	sigprocmask(SIG_SETMASK, &oldset, NULL);
 	tsk_restore_flags(current, pflags, PF_MEMALLOC);
 
 	if (!send && nbd->xmit_timeout)
@@ -406,23 +404,18 @@ 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;
 	}
@@ -439,19 +432,7 @@ 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)) {
-		ret = kernel_dequeue_signal(NULL);
-		dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal %d\n",
-			 task_pid_nr(current), current->comm, ret);
-		mutex_lock(&nbd->tx_lock);
-		sock_shutdown(nbd);
-		mutex_unlock(&nbd->tx_lock);
-		ret = -ETIMEDOUT;
-	}
 
 	return ret;
 }
@@ -544,11 +525,8 @@ 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)) {
@@ -557,17 +535,6 @@ static int nbd_thread_send(void *data)
 					 kthread_should_stop() ||
 					 !list_empty(&nbd->waiting_queue));
 
-		if (signal_pending(current)) {
-			int ret = kernel_dequeue_signal(NULL);
-
-			dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal %d\n",
-				 task_pid_nr(current), current->comm, ret);
-			mutex_lock(&nbd->tx_lock);
-			sock_shutdown(nbd);
-			mutex_unlock(&nbd->tx_lock);
-			break;
-		}
-
 		/* extract request */
 		if (list_empty(&nbd->waiting_queue))
 			continue;
@@ -582,13 +549,7 @@ 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))
-		kernel_dequeue_signal(NULL);
 
 	return 0;
 }
@@ -636,6 +597,25 @@ static void nbd_request_handler(struct request_queue *q)
 	}
 }
 
+static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
+{
+	int ret = 0;
+
+	spin_lock_irq(&nbd->sock_lock);
+
+	if (nbd->sock) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	nbd->sock = sock;
+
+out:
+	spin_unlock_irq(&nbd->sock_lock);
+
+	return ret;
+}
+
 static int nbd_dev_dbg_init(struct nbd_device *nbd);
 static void nbd_dev_dbg_close(struct nbd_device *nbd);
 
@@ -668,32 +648,22 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		return 0;
 	}
  
-	case NBD_CLEAR_SOCK: {
-		struct socket *sock = nbd->sock;
-		nbd->sock = NULL;
+	case NBD_CLEAR_SOCK:
+		sock_shutdown(nbd);
 		nbd_clear_que(nbd);
 		BUG_ON(!list_empty(&nbd->queue_head));
 		BUG_ON(!list_empty(&nbd->waiting_queue));
 		kill_bdev(bdev);
-		if (sock)
-			sockfd_put(sock);
 		return 0;
-	}
 
 	case NBD_SET_SOCK: {
-		struct socket *sock;
 		int err;
-		if (nbd->sock)
-			return -EBUSY;
-		sock = sockfd_lookup(arg, &err);
-		if (sock) {
-			nbd->sock = sock;
-			if (max_part > 0)
-				bdev->bd_invalidated = 1;
-			nbd->disconnect = false; /* we're connected now */
-			return 0;
-		}
-		return -EINVAL;
+		struct socket *sock = sockfd_lookup(arg, &err);
+
+		if (!sock)
+			return err;
+
+		return nbd_set_socket(nbd, sock);
 	}
 
 	case NBD_SET_BLKSIZE:
@@ -734,7 +704,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 
 	case NBD_DO_IT: {
 		struct task_struct *thread;
-		struct socket *sock;
 		int error;
 
 		if (nbd->task_recv)
@@ -769,14 +738,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		mutex_lock(&nbd->tx_lock);
 
 		sock_shutdown(nbd);
-		sock = nbd->sock;
-		nbd->sock = NULL;
 		nbd_clear_que(nbd);
 		kill_bdev(bdev);
 		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
 		set_device_ro(bdev, false);
-		if (sock)
-			sockfd_put(sock);
 		nbd->flags = 0;
 		nbd->bytesize = 0;
 		bdev->bd_inode->i_size = 0;
@@ -1043,7 +1008,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);
+		spin_lock_init(&nbd_dev[i].sock_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.1

[PATCH 2/4] nbd: Timeouts are not user requested disconnects

[Markus Pargmann]

It may be useful to know in the client that a connection timed out. The
current code returns success for a timeout.

This patch reports the error code -ETIMEDOUT for a timeout.

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index d14102d1e9ba..0a87947f94db 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -57,6 +57,7 @@ struct nbd_device {
 	int blksize;
 	loff_t bytesize;
 	int xmit_timeout;
+	bool timedout;
 	bool disconnect; /* a disconnect has been requested by user */
 
 	struct timer_list timeout_timer;
@@ -153,10 +154,9 @@ static void nbd_xmit_timeout(unsigned long arg)
 	if (list_empty(&nbd->queue_head))
 		return;
 
-	nbd->disconnect = true;
-
 	spin_lock_irqsave(&nbd->sock_lock, flags);
 
+	nbd->timedout = true;
 
 	if (nbd->sock)
 		kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
@@ -749,7 +749,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		if (max_part > 0)
 			blkdev_reread_part(bdev);
 		if (nbd->disconnect) /* user requested, ignore socket errors */
-			return 0;
+			error = 0;
+		if (nbd->timedout)
+			error = -ETIMEDOUT;
+
 		return error;
 	}
 
-- 
2.6.1

[PATCH 3/4] nbd: Cleanup reset of nbd and bdev after a disconnect

[Markus Pargmann]

Group all variables that are reset after a disconnect into reset
functions. This patch adds two of these functions, nbd_reset() and
nbd_bdev_reset().

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0a87947f94db..8ba142b4cfe1 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -616,6 +616,29 @@ out:
 	return ret;
 }
 
+/* Reset all properties of an NBD device */
+static void nbd_reset(struct nbd_device *nbd)
+{
+	nbd->disconnect = false;
+	nbd->timedout = false;
+	nbd->blksize = 1024;
+	nbd->bytesize = 0;
+	set_capacity(nbd->disk, 0);
+	nbd->flags = 0;
+	nbd->xmit_timeout = 0;
+	queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
+}
+
+static void nbd_bdev_reset(struct block_device *bdev)
+{
+	set_device_ro(bdev, false);
+	bdev->bd_inode->i_size = 0;
+	if (max_part > 0) {
+		blkdev_reread_part(bdev);
+		bdev->bd_invalidated = 1;
+	}
+}
+
 static int nbd_dev_dbg_init(struct nbd_device *nbd);
 static void nbd_dev_dbg_close(struct nbd_device *nbd);
 
@@ -740,19 +763,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		sock_shutdown(nbd);
 		nbd_clear_que(nbd);
 		kill_bdev(bdev);
-		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
-		set_device_ro(bdev, false);
-		nbd->flags = 0;
-		nbd->bytesize = 0;
-		bdev->bd_inode->i_size = 0;
-		set_capacity(nbd->disk, 0);
-		if (max_part > 0)
-			blkdev_reread_part(bdev);
+		nbd_bdev_reset(bdev);
+
 		if (nbd->disconnect) /* user requested, ignore socket errors */
 			error = 0;
 		if (nbd->timedout)
 			error = -ETIMEDOUT;
 
+		nbd_reset(nbd);
+
 		return error;
 	}
 
@@ -1019,14 +1038,12 @@ static int __init nbd_init(void)
 		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;
-		nbd_dev[i].bytesize = 0;
 		disk->major = NBD_MAJOR;
 		disk->first_minor = i << part_shift;
 		disk->fops = &nbd_fops;
 		disk->private_data = &nbd_dev[i];
 		sprintf(disk->disk_name, "nbd%d", i);
-		set_capacity(disk, 0);
+		nbd_reset(&nbd_dev[i]);
 		add_disk(disk);
 	}
 
-- 
2.6.1

[PATCH 4/4] nbd: Move flag parsing to a function

[Markus Pargmann]

nbd changes properties of the blockdevice depending on flags that were
received. This patch moves this flag parsing into a separate function
nbd_parse_flags().

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 8ba142b4cfe1..205cfc404bc0 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -639,6 +639,18 @@ static void nbd_bdev_reset(struct block_device *bdev)
 	}
 }
 
+static void nbd_parse_flags(struct nbd_device *nbd, struct block_device *bdev)
+{
+	if (nbd->flags & NBD_FLAG_READ_ONLY)
+		set_device_ro(bdev, true);
+	if (nbd->flags & NBD_FLAG_SEND_TRIM)
+		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
+	if (nbd->flags & NBD_FLAG_SEND_FLUSH)
+		blk_queue_flush(nbd->disk->queue, REQ_FLUSH);
+	else
+		blk_queue_flush(nbd->disk->queue, 0);
+}
+
 static int nbd_dev_dbg_init(struct nbd_device *nbd);
 static void nbd_dev_dbg_close(struct nbd_device *nbd);
 
@@ -736,15 +748,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 
 		mutex_unlock(&nbd->tx_lock);
 
-		if (nbd->flags & NBD_FLAG_READ_ONLY)
-			set_device_ro(bdev, true);
-		if (nbd->flags & NBD_FLAG_SEND_TRIM)
-			queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
-				nbd->disk->queue);
-		if (nbd->flags & NBD_FLAG_SEND_FLUSH)
-			blk_queue_flush(nbd->disk->queue, REQ_FLUSH);
-		else
-			blk_queue_flush(nbd->disk->queue, 0);
+		nbd_parse_flags(nbd, bdev);
 
 		thread = kthread_run(nbd_thread_send, nbd, "%s",
 				     nbd_name(nbd));
-- 
2.6.1

Re: [PATCH 0/4] nbd: Remove signal usage

[Oleg Nesterov]

Hi Markus,

Sorry again for delay. I was offlist. again.

On 10/29, Markus Pargmann wrote:
>
> Hi,
>
> this is a try to remove all the signals from NBD. The first patch replaces the
> signals. The other patches are some cleanups I made on the way.
>
> This should solve the kthread_run() problems as well.

I obviously can't review these changes, I do not understand this code
enough. But they look good imo.

However, I do not understand the usage of ->task_recv and ->task_send.

pid_show() doesn't even check nbd->task_recv != NULL. Honestly, I simply
do not know if it can race with device_remove_file() or not. I think it
can, but I can be easily wrong...

nbd_dbg_tasks_show() looks racy too even if it checks task_recv/task_send,
at least this needs READ_ONCE() but in theory this is not enough,
task_pid_nr() can read the freed task_struct.

Again, I can easily miss something. But whatever I missed, perhaps the
trivial (but uncompiled/untested) patch below makes sense anyway?

Oleg.


diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index f547005..67c1e09 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -63,8 +63,8 @@ struct nbd_device {
 	struct timer_list timeout_timer;
 	/* protects initialization and shutdown of the socket */
 	spinlock_t sock_lock;
-	struct task_struct *task_recv;
-	struct task_struct *task_send;
+	pid_t task_recv;
+	pid_t task_send;
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 	struct dentry *dbg_dir;
@@ -392,7 +392,7 @@ static ssize_t pid_show(struct device *dev,
 	struct gendisk *disk = dev_to_disk(dev);
 	struct nbd_device *nbd = (struct nbd_device *)disk->private_data;
 
-	return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv));
+	return sprintf(buf, "%d\n", nbd->task_recv);
 }
 
 static struct device_attribute pid_attr = {
@@ -409,13 +409,13 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 
 	sk_set_memalloc(nbd->sock->sk);
 
-	nbd->task_recv = current;
+	nbd->task_recv = task_pid_nr(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->task_recv = NULL;
+		nbd->task_recv = 0;
 
 		return ret;
 	}
@@ -432,7 +432,7 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 
 	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 
-	nbd->task_recv = NULL;
+	nbd->task_recv = 0;
 
 	return ret;
 }
@@ -526,7 +526,7 @@ static int nbd_thread_send(void *data)
 	struct nbd_device *nbd = data;
 	struct request *req;
 
-	nbd->task_send = current;
+	nbd->task_send = task_pid_nr(current);
 
 	set_user_nice(current, MIN_NICE);
 	while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {
@@ -549,7 +549,7 @@ static int nbd_thread_send(void *data)
 		nbd_handle_req(nbd, req);
 	}
 
-	nbd->task_send = NULL;
+	nbd->task_send = 0;
 
 	return 0;
 }
@@ -827,9 +827,9 @@ 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));
+		seq_printf(s, "recv: %d\n", nbd->task_recv);
 	if (nbd->task_send)
-		seq_printf(s, "send: %d\n", task_pid_nr(nbd->task_send));
+		seq_printf(s, "send: %d\n", nbd->task_send);
 
 	return 0;
 }

Re: [PATCH 0/4] nbd: Remove signal usage

[Markus Pargmann]

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

Hi Oleg,

On Sunday 01 November 2015 20:05:00 Oleg Nesterov wrote:
> Hi Markus,
> 
> Sorry again for delay. I was offlist. again.

Sorry I hadn't too much time last week.

> 
> On 10/29, Markus Pargmann wrote:
> >
> > Hi,
> >
> > this is a try to remove all the signals from NBD. The first patch replaces the
> > signals. The other patches are some cleanups I made on the way.
> >
> > This should solve the kthread_run() problems as well.
> 
> I obviously can't review these changes, I do not understand this code
> enough. But they look good imo.

Thanks for having a look.

> 
> However, I do not understand the usage of ->task_recv and ->task_send.
> 
> pid_show() doesn't even check nbd->task_recv != NULL. Honestly, I simply
> do not know if it can race with device_remove_file() or not. I think it
> can, but I can be easily wrong...

pid_show() should hopefully not be the problem. The 'pid' file which uses
pid_show() is created after task_recv was set and is removed using
device_remove_file(). Assuming that there are no open calls to pid_show() after
device_remove_file() was called this setup should not have a race issue.

> 
> nbd_dbg_tasks_show() looks racy too even if it checks task_recv/task_send,
> at least this needs READ_ONCE() but in theory this is not enough,
> task_pid_nr() can read the freed task_struct.

Yes it requires a READ_ONCE(). But it is not possible for task_pid_nr to use
the freed task_struct. With the patches I posted, the send thread is kept alive
until kthread_stop() is called. The debugfs files are removed before the send
thread is stopped. So if there is a task struct it is valid.

> 
> Again, I can easily miss something. But whatever I missed, perhaps the
> trivial (but uncompiled/untested) patch below makes sense anyway?

Yes as we don't need the task_struct anymore to send signals it makes sense.

Best Regards,

Markus

> 
> Oleg.
> 
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index f547005..67c1e09 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -63,8 +63,8 @@ struct nbd_device {
>  	struct timer_list timeout_timer;
>  	/* protects initialization and shutdown of the socket */
>  	spinlock_t sock_lock;
> -	struct task_struct *task_recv;
> -	struct task_struct *task_send;
> +	pid_t task_recv;
> +	pid_t task_send;
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>  	struct dentry *dbg_dir;
> @@ -392,7 +392,7 @@ static ssize_t pid_show(struct device *dev,
>  	struct gendisk *disk = dev_to_disk(dev);
>  	struct nbd_device *nbd = (struct nbd_device *)disk->private_data;
>  
> -	return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv));
> +	return sprintf(buf, "%d\n", nbd->task_recv);
>  }
>  
>  static struct device_attribute pid_attr = {
> @@ -409,13 +409,13 @@ static int nbd_thread_recv(struct nbd_device *nbd)
>  
>  	sk_set_memalloc(nbd->sock->sk);
>  
> -	nbd->task_recv = current;
> +	nbd->task_recv = task_pid_nr(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->task_recv = NULL;
> +		nbd->task_recv = 0;
>  
>  		return ret;
>  	}
> @@ -432,7 +432,7 @@ static int nbd_thread_recv(struct nbd_device *nbd)
>  
>  	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
>  
> -	nbd->task_recv = NULL;
> +	nbd->task_recv = 0;
>  
>  	return ret;
>  }
> @@ -526,7 +526,7 @@ static int nbd_thread_send(void *data)
>  	struct nbd_device *nbd = data;
>  	struct request *req;
>  
> -	nbd->task_send = current;
> +	nbd->task_send = task_pid_nr(current);
>  
>  	set_user_nice(current, MIN_NICE);
>  	while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {
> @@ -549,7 +549,7 @@ static int nbd_thread_send(void *data)
>  		nbd_handle_req(nbd, req);
>  	}
>  
> -	nbd->task_send = NULL;
> +	nbd->task_send = 0;
>  
>  	return 0;
>  }
> @@ -827,9 +827,9 @@ 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));
> +		seq_printf(s, "recv: %d\n", nbd->task_recv);
>  	if (nbd->task_send)
> -		seq_printf(s, "send: %d\n", task_pid_nr(nbd->task_send));
> +		seq_printf(s, "send: %d\n", nbd->task_send);
>  
>  	return 0;
>  }
> 
> 

-- 
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: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/4] nbd: Remove signal usage

[Al Viro]

On Thu, Oct 29, 2015 at 04:42:37PM +0100, Markus Pargmann wrote:
>  	del_timer_sync(&nbd->timeout_timer);
> +
> +out:
> +	spin_unlock_irq(&nbd->sock_lock);

... and in its callback we have this:

> @@ -148,17 +155,15 @@ static void nbd_xmit_timeout(unsigned long arg)
>  
>  	nbd->disconnect = true;
>  
> -	spin_lock_irqsave(&nbd->tasks_lock, flags);
> +	spin_lock_irqsave(&nbd->sock_lock, flags);

* CPU 1 enters sock_shutdown() and grabs ->sock_lock.
* on CPU2 the timer hits and we enter the callback, where we spin on that
spinlock.
* in the meanwhile, CPU1 calls del_timer_sync()

Deadlock...

Re: [PATCH 1/4] nbd: Remove signal usage

[Markus Pargmann]

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

Hi,

On Tuesday 10 November 2015 04:46:18 Al Viro wrote:
> On Thu, Oct 29, 2015 at 04:42:37PM +0100, Markus Pargmann wrote:
> >  	del_timer_sync(&nbd->timeout_timer);
> > +
> > +out:
> > +	spin_unlock_irq(&nbd->sock_lock);
> 
> ... and in its callback we have this:
> 
> > @@ -148,17 +155,15 @@ static void nbd_xmit_timeout(unsigned long arg)
> >  
> >  	nbd->disconnect = true;
> >  
> > -	spin_lock_irqsave(&nbd->tasks_lock, flags);
> > +	spin_lock_irqsave(&nbd->sock_lock, flags);
> 
> * CPU 1 enters sock_shutdown() and grabs ->sock_lock.
> * on CPU2 the timer hits and we enter the callback, where we spin on that
> spinlock.
> * in the meanwhile, CPU1 calls del_timer_sync()
> 
> Deadlock...

Thank you. Yes that locking block in sock_shutdown is to large. And probably
the del_timer_sync() isn't necessary, we can just use del_timer().

It may even be possible to remove the sock_lock completely. Will look into this
and post a v2.

Thanks,

Markus

-- 
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: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]