Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1240834 > unrolled thread

[PATCH] nbd: Add locking for tasks

Started byMarkus Pargmann <mpa@pengutronix.de>
First post2015-10-06 20:10 +0200
Last post2015-10-08 22:30 +0200
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] nbd: Add locking for tasks Markus Pargmann <mpa@pengutronix.de> - 2015-10-06 20:10 +0200
    Re: [PATCH] nbd: Add locking for tasks Ben Hutchings <ben@decadent.org.uk> - 2015-10-08 22:20 +0200
      Re: [PATCH] nbd: Add locking for tasks Jens Axboe <axboe@kernel.dk> - 2015-10-08 22:30 +0200

#1240834 — [PATCH] nbd: Add locking for tasks

FromMarkus Pargmann <mpa@pengutronix.de>
Date2015-10-06 20:10 +0200
Subject[PATCH] nbd: Add locking for tasks
Message-ID<qgEQy-5kz-33@gated-at.bofh.it>
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1242715

FromBen Hutchings <ben@decadent.org.uk>
Date2015-10-08 22:20 +0200
Message-ID<qhpPr-5yd-7@gated-at.bofh.it>
In reply to#1240834

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1242722

FromJens Axboe <axboe@kernel.dk>
Date2015-10-08 22:30 +0200
Message-ID<qhpZ8-5Jz-11@gated-at.bofh.it>
In reply to#1242715
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web