[PATCH 0/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl()

[PATCH 0/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl()

[Oleg Nesterov]

Untested. Needs an ack from Markus, but unless I missed something
this is v4.3 material.

Oleg.

[PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl()

[Oleg Nesterov]

It is not safe to use the task_struct returned by kthread_run(threadfn)
if threadfn() can exit before the "owner" does kthread_stop(), nothing
protects this task_struct.

So __nbd_ioctl() looks buggy; a killed nbd_thread_send() can exit, free
its task_struct, and then kthread_stop() can use the freed/reused memory.

Add the new trivial helper, kthread_get_run(). Hopefully it will have more
users, this patch changes __nbd_ioctl() as an example.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 drivers/block/nbd.c     |    5 +++--
 include/linux/kthread.h |   12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 93b3f99..b85e7a0 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -754,8 +754,8 @@ 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_send, nbd, "%s",
-				     nbd_name(nbd));
+		thread = kthread_get_run(nbd_thread_send, nbd, "%s",
+					 nbd_name(nbd));
 		if (IS_ERR(thread)) {
 			mutex_lock(&nbd->tx_lock);
 			return PTR_ERR(thread);
@@ -765,6 +765,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		error = nbd_thread_recv(nbd);
 		nbd_dev_dbg_close(nbd);
 		kthread_stop(thread);
+		put_task_struct(thread);
 
 		mutex_lock(&nbd->tx_lock);
 
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 13d5520..b0465cc 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -37,6 +37,18 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 	__k;								   \
 })
 
+/* Same as kthread_run() but also pin the task_struct */
+#define kthread_get_run(threadfn, data, namefmt, ...)			   \
+({									   \
+	struct task_struct *__k						   \
+		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k)) {						   \
+		get_task_struct(__k);					   \
+		wake_up_process(__k);					   \
+	}								   \
+	__k;								   \
+})
+
 void kthread_bind(struct task_struct *k, unsigned int cpu);
 void kthread_bind_mask(struct task_struct *k, const struct cpumask *mask);
 int kthread_stop(struct task_struct *k);
-- 
1.5.5.1

Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl()

[Markus Pargmann]

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

On Sun, Oct 25, 2015 at 03:27:13PM +0100, Oleg Nesterov wrote:
> It is not safe to use the task_struct returned by kthread_run(threadfn)
> if threadfn() can exit before the "owner" does kthread_stop(), nothing
> protects this task_struct.
> 
> So __nbd_ioctl() looks buggy; a killed nbd_thread_send() can exit, free
> its task_struct, and then kthread_stop() can use the freed/reused memory.
> 
> Add the new trivial helper, kthread_get_run(). Hopefully it will have more
> users, this patch changes __nbd_ioctl() as an example.

Thanks.

Acked-by: Markus Pargmann <mpa@pengutronix.de>

However I am not sure this is important for 4.3 final. This bug is
present since at least 2008 (didn't look further).

Best Regards,

Markus

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  drivers/block/nbd.c     |    5 +++--
>  include/linux/kthread.h |   12 ++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 93b3f99..b85e7a0 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -754,8 +754,8 @@ 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_send, nbd, "%s",
> -				     nbd_name(nbd));
> +		thread = kthread_get_run(nbd_thread_send, nbd, "%s",
> +					 nbd_name(nbd));
>  		if (IS_ERR(thread)) {
>  			mutex_lock(&nbd->tx_lock);
>  			return PTR_ERR(thread);
> @@ -765,6 +765,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>  		error = nbd_thread_recv(nbd);
>  		nbd_dev_dbg_close(nbd);
>  		kthread_stop(thread);
> +		put_task_struct(thread);
>  
>  		mutex_lock(&nbd->tx_lock);
>  
> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index 13d5520..b0465cc 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -37,6 +37,18 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
>  	__k;								   \
>  })
>  
> +/* Same as kthread_run() but also pin the task_struct */
> +#define kthread_get_run(threadfn, data, namefmt, ...)			   \
> +({									   \
> +	struct task_struct *__k						   \
> +		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
> +	if (!IS_ERR(__k)) {						   \
> +		get_task_struct(__k);					   \
> +		wake_up_process(__k);					   \
> +	}								   \
> +	__k;								   \
> +})
> +
>  void kthread_bind(struct task_struct *k, unsigned int cpu);
>  void kthread_bind_mask(struct task_struct *k, const struct cpumask *mask);
>  int kthread_stop(struct task_struct *k);
> -- 
> 1.5.5.1
> 
> 
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl()

[Christoph Hellwig]

On Sun, Oct 25, 2015 at 03:27:13PM +0100, Oleg Nesterov wrote:
> It is not safe to use the task_struct returned by kthread_run(threadfn)
> if threadfn() can exit before the "owner" does kthread_stop(), nothing
> protects this task_struct.
> 
> So __nbd_ioctl() looks buggy; a killed nbd_thread_send() can exit, free
> its task_struct, and then kthread_stop() can use the freed/reused memory.
> 
> Add the new trivial helper, kthread_get_run(). Hopefully it will have more
> users, this patch changes __nbd_ioctl() as an example.

This looks horrible.  I think the real problem is that nbd is totally
abusing signals for kthreads and that needs to go away.

Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl()

[Markus Pargmann]

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

Hi,

On Mon, Oct 26, 2015 at 05:26:42PM -0700, Christoph Hellwig wrote:
> On Sun, Oct 25, 2015 at 03:27:13PM +0100, Oleg Nesterov wrote:
> > It is not safe to use the task_struct returned by kthread_run(threadfn)
> > if threadfn() can exit before the "owner" does kthread_stop(), nothing
> > protects this task_struct.
> > 
> > So __nbd_ioctl() looks buggy; a killed nbd_thread_send() can exit, free
> > its task_struct, and then kthread_stop() can use the freed/reused memory.
> > 
> > Add the new trivial helper, kthread_get_run(). Hopefully it will have more
> > users, this patch changes __nbd_ioctl() as an example.
> 
> This looks horrible.  I think the real problem is that nbd is totally
> abusing signals for kthreads and that needs to go away.

To avoid this kthread_get_run() we can change the NBD code as well to
guarantee that the thread does not exit until kthread_stop() was called.
I think that is independent of using signals.

Currently NBD uses signals for the timeout handling to get the threads
out of the TCP operations. Do you have an idea how to solve this
differently?

Best Regards,

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl()

[Oleg Nesterov]

Markus,

sorry for delay, I didn't have email access two days,

On 10/26, Markus Pargmann wrote:
>
> On Sun, Oct 25, 2015 at 03:27:13PM +0100, Oleg Nesterov wrote:
> > It is not safe to use the task_struct returned by kthread_run(threadfn)
> > if threadfn() can exit before the "owner" does kthread_stop(), nothing
> > protects this task_struct.
> >
> > So __nbd_ioctl() looks buggy; a killed nbd_thread_send() can exit, free
> > its task_struct, and then kthread_stop() can use the freed/reused memory.
> >
> > Add the new trivial helper, kthread_get_run(). Hopefully it will have more
> > users, this patch changes __nbd_ioctl() as an example.
>
> Thanks.
>
> Acked-by: Markus Pargmann <mpa@pengutronix.de>
>
> However I am not sure this is important for 4.3 final. This bug is
> present since at least 2008 (didn't look further).

Ah yes, I din't bother to check the history of this code, thanks.

So this bug is very old, no need to push the fix into 4.3.

Oleg.

Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl()

[Oleg Nesterov]

On 10/26, Christoph Hellwig wrote:
>
> On Sun, Oct 25, 2015 at 03:27:13PM +0100, Oleg Nesterov wrote:
> > It is not safe to use the task_struct returned by kthread_run(threadfn)
> > if threadfn() can exit before the "owner" does kthread_stop(), nothing
> > protects this task_struct.
> >
> > So __nbd_ioctl() looks buggy; a killed nbd_thread_send() can exit, free
> > its task_struct, and then kthread_stop() can use the freed/reused memory.
> >
> > Add the new trivial helper, kthread_get_run(). Hopefully it will have more
> > users, this patch changes __nbd_ioctl() as an example.
>
> This looks horrible.

Do you mean the helper itself?

In fact iirc people asked for this helper before. It looks natural and simple.
kthread_run() can only be used if this kthread can't exit on its own.

> I think the real problem is that nbd is totally
> abusing signals for kthreads and that needs to go away.

I agree this code needs cleanups. And of course we can fix it without
new helper, but see above.

Oleg.