* [PATCH 0/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() @ 2015-10-25 14:26 Oleg Nesterov 2015-10-25 14:27 ` [PATCH 1/1] " Oleg Nesterov 0 siblings, 1 reply; 7+ messages in thread From: Oleg Nesterov @ 2015-10-25 14:26 UTC (permalink / raw) To: Andrew Morton, Markus Pargmann; +Cc: Tejun Heo, nbd-general, linux-kernel Untested. Needs an ack from Markus, but unless I missed something this is v4.3 material. Oleg. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() 2015-10-25 14:26 [PATCH 0/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() Oleg Nesterov @ 2015-10-25 14:27 ` Oleg Nesterov 2015-10-26 7:33 ` Markus Pargmann 2015-10-27 0:26 ` Christoph Hellwig 0 siblings, 2 replies; 7+ messages in thread From: Oleg Nesterov @ 2015-10-25 14:27 UTC (permalink / raw) To: Andrew Morton, Markus Pargmann; +Cc: Tejun Heo, nbd-general, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() 2015-10-25 14:27 ` [PATCH 1/1] " Oleg Nesterov @ 2015-10-26 7:33 ` Markus Pargmann 2015-10-28 15:27 ` Oleg Nesterov 2015-10-27 0:26 ` Christoph Hellwig 1 sibling, 1 reply; 7+ messages in thread From: Markus Pargmann @ 2015-10-26 7:33 UTC (permalink / raw) To: Oleg Nesterov; +Cc: Andrew Morton, Tejun Heo, nbd-general, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() 2015-10-26 7:33 ` Markus Pargmann @ 2015-10-28 15:27 ` Oleg Nesterov 0 siblings, 0 replies; 7+ messages in thread From: Oleg Nesterov @ 2015-10-28 15:27 UTC (permalink / raw) To: Markus Pargmann; +Cc: Andrew Morton, Tejun Heo, nbd-general, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() 2015-10-25 14:27 ` [PATCH 1/1] " Oleg Nesterov 2015-10-26 7:33 ` Markus Pargmann @ 2015-10-27 0:26 ` Christoph Hellwig 2015-10-27 7:03 ` Markus Pargmann 2015-10-28 15:37 ` Oleg Nesterov 1 sibling, 2 replies; 7+ messages in thread From: Christoph Hellwig @ 2015-10-27 0:26 UTC (permalink / raw) To: Oleg Nesterov Cc: Andrew Morton, Markus Pargmann, Tejun Heo, nbd-general, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() 2015-10-27 0:26 ` Christoph Hellwig @ 2015-10-27 7:03 ` Markus Pargmann 2015-10-28 15:37 ` Oleg Nesterov 1 sibling, 0 replies; 7+ messages in thread From: Markus Pargmann @ 2015-10-27 7:03 UTC (permalink / raw) To: Christoph Hellwig Cc: Oleg Nesterov, Andrew Morton, Tejun Heo, nbd-general, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() 2015-10-27 0:26 ` Christoph Hellwig 2015-10-27 7:03 ` Markus Pargmann @ 2015-10-28 15:37 ` Oleg Nesterov 1 sibling, 0 replies; 7+ messages in thread From: Oleg Nesterov @ 2015-10-28 15:37 UTC (permalink / raw) To: Christoph Hellwig Cc: Andrew Morton, Markus Pargmann, Tejun Heo, nbd-general, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-10-28 14:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-25 14:26 [PATCH 0/1] kthread: introduce kthread_get_run() to fix __nbd_ioctl() Oleg Nesterov 2015-10-25 14:27 ` [PATCH 1/1] " Oleg Nesterov 2015-10-26 7:33 ` Markus Pargmann 2015-10-28 15:27 ` Oleg Nesterov 2015-10-27 0:26 ` Christoph Hellwig 2015-10-27 7:03 ` Markus Pargmann 2015-10-28 15:37 ` Oleg Nesterov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).