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

Groups > linux.kernel > #1260213 > unrolled thread

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

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

  Re: [PATCH 0/4] nbd: Remove signal usage Oleg Nesterov <oleg@redhat.com> - 2015-11-01 19:10 +0100
    Re: [PATCH 0/4] nbd: Remove signal usage Markus Pargmann <mpa@pengutronix.de> - 2015-11-09 12:30 +0100

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

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;
 }

--
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]

#1265592

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

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 |

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web