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

Groups > linux.kernel > #1255395 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/1] kthread: introduce kthread_get_run() to fix  __nbd_ioctl() Oleg Nesterov <oleg@redhat.com> - 2015-10-25 14:40 +0100
    [PATCH 1/1] kthread: introduce kthread_get_run() to fix  __nbd_ioctl() Oleg Nesterov <oleg@redhat.com> - 2015-10-25 14:40 +0100
      Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix  __nbd_ioctl() Markus Pargmann <mpa@pengutronix.de> - 2015-10-26 08:40 +0100
        Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix  __nbd_ioctl() Oleg Nesterov <oleg@redhat.com> - 2015-10-28 15:40 +0100
      Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix  __nbd_ioctl() Christoph Hellwig <hch@infradead.org> - 2015-10-27 01:30 +0100
        Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix  __nbd_ioctl() Markus Pargmann <mpa@pengutronix.de> - 2015-10-27 08:10 +0100
        Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix  __nbd_ioctl() Oleg Nesterov <oleg@redhat.com> - 2015-10-28 15:50 +0100

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

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

Oleg.

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

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

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


--
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] | [next] | [standalone]

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

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

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 |

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

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

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.

--
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] | [next] | [standalone]

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

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 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] | [next] | [standalone]

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

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

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 |

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

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

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.

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