kthread: introduce kthread_get_run() to fix __nbd_ioctl()
Subsystems (2)
cover 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.
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.
--- drivers/block/nbd.c | 5 +++-- include/linux/kthread.h | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-)
drivers/block/nbd.c +3 -2
@@ -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);
include/linux/kthread.h +12 -0
@@ -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);
Signature · 1 line
1.5.5.1
Quoted 9 lines · Oleg Nesterov
It is not safe to use the
task_structreturned bykthread_run(threadfn)ifthreadfn()can exit before the "owner" doeskthread_stop(), nothing protects thistask_struct.So
__nbd_ioctl()looks buggy; a killednbd_thread_send()can exit, free itstask_struct, and thenkthread_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.
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
Quoted 58 lines
Signed-off-by Oleg Nesterov <[email protected]>--- drivers/block/nbd.c | 5 +++-- include/linux/kthread.h | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-)drivers/block/nbd.c +3
-2@@ -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);include/linux/kthread.h +12
-0@@ -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);Signature · 1 line
1.5.5.1
Signature · 4 lines
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 |
Quoted 9 lines · Oleg Nesterov
It is not safe to use the
task_structreturned bykthread_run(threadfn)ifthreadfn()can exit before the "owner" doeskthread_stop(), nothing protects thistask_struct.So
__nbd_ioctl()looks buggy; a killednbd_thread_send()can exit, free itstask_struct, and thenkthread_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.
Hi,
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Oleg Nesterov
It is not safe to use the
task_structreturned bykthread_run(threadfn)ifthreadfn()can exit before the "owner" doeskthread_stop(), nothing protects thistask_struct.
Quoted 8 lines
Quoted 5 lines
So
__nbd_ioctl()looks buggy; a killednbd_thread_send()can exit, free itstask_struct, and thenkthread_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
Signature · 4 lines
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 |
Markus,
sorry for delay, I didn't have email access two days,
Quoted 12 lines · Markus Pargmann
Quoted 9 lines · Oleg Nesterov
It is not safe to use the
task_structreturned bykthread_run(threadfn)ifthreadfn()can exit before the "owner" doeskthread_stop(), nothing protects thistask_struct.So
__nbd_ioctl()looks buggy; a killednbd_thread_send()can exit, free itstask_struct, and thenkthread_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.
Quoted 6 lines
Thanks.
Acked-by Markus Pargmann <[email protected]>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.
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines · Oleg Nesterov
It is not safe to use the
task_structreturned bykthread_run(threadfn)ifthreadfn()can exit before the "owner" doeskthread_stop(), nothing protects thistask_struct.
Quoted 7 lines
Quoted 5 lines
So
__nbd_ioctl()looks buggy; a killednbd_thread_send()can exit, free itstask_struct, and thenkthread_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.
Quoted 2 lines
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.