* [PATCH] ehca: fix kthread_create() error check
@ 2006-12-19 8:42 Akinobu Mita
2006-12-19 9:32 ` Hoang-Nam Nguyen
2006-12-21 21:22 ` Heiko Carstens
0 siblings, 2 replies; 11+ messages in thread
From: Akinobu Mita @ 2006-12-19 8:42 UTC (permalink / raw)
To: linux-kernel; +Cc: Hoang-Nam Nguyen, Christoph Raisch
The return value of kthread_create() should be checked by
IS_ERR(). create_comp_task() returns the return value from
kthread_create().
Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
Cc: Christoph Raisch <raisch@de.ibm.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
drivers/infiniband/hw/ehca/ehca_irq.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
===================================================================
--- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
+++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
@@ -670,11 +670,13 @@ static int comp_pool_callback(struct not
{
unsigned int cpu = (unsigned long)hcpu;
struct ehca_cpu_comp_task *cct;
+ struct task_struct *task;
switch (action) {
case CPU_UP_PREPARE:
ehca_gen_dbg("CPU: %x (CPU_PREPARE)", cpu);
- if(!create_comp_task(pool, cpu)) {
+ task = create_comp_task(pool, cpu);
+ if (IS_ERR(task)) {
ehca_gen_err("Can't create comp_task for cpu: %x", cpu);
return NOTIFY_BAD;
}
@@ -730,7 +732,7 @@ int ehca_create_comp_pool(void)
for_each_online_cpu(cpu) {
task = create_comp_task(pool, cpu);
- if (task) {
+ if (!IS_ERR(task)) {
kthread_bind(task, cpu);
wake_up_process(task);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ehca: fix kthread_create() error check
2006-12-19 8:42 [PATCH] ehca: fix kthread_create() error check Akinobu Mita
@ 2006-12-19 9:32 ` Hoang-Nam Nguyen
2006-12-21 21:22 ` Heiko Carstens
1 sibling, 0 replies; 11+ messages in thread
From: Hoang-Nam Nguyen @ 2006-12-19 9:32 UTC (permalink / raw)
To: Akinobu Mita; +Cc: Christoph Raisch, linux-kernel
Hi,
> The return value of kthread_create() should be checked by
> IS_ERR(). create_comp_task() returns the return value from
> kthread_create().
Good catch. Appreciate your help!
Regards
Nam
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ehca: fix kthread_create() error check
2006-12-19 8:42 [PATCH] ehca: fix kthread_create() error check Akinobu Mita
2006-12-19 9:32 ` Hoang-Nam Nguyen
@ 2006-12-21 21:22 ` Heiko Carstens
2006-12-25 8:12 ` [PATCH -mm] ehca: avoid crash on kthread_create() failure Akinobu Mita
` (2 more replies)
1 sibling, 3 replies; 11+ messages in thread
From: Heiko Carstens @ 2006-12-21 21:22 UTC (permalink / raw)
To: Akinobu Mita, linux-kernel, Hoang-Nam Nguyen, Christoph Raisch
> Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> ===================================================================
> --- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
> +++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> @@ -670,11 +670,13 @@ static int comp_pool_callback(struct not
> {
> unsigned int cpu = (unsigned long)hcpu;
> struct ehca_cpu_comp_task *cct;
> + struct task_struct *task;
>
> switch (action) {
> case CPU_UP_PREPARE:
> ehca_gen_dbg("CPU: %x (CPU_PREPARE)", cpu);
> - if(!create_comp_task(pool, cpu)) {
> + task = create_comp_task(pool, cpu);
> + if (IS_ERR(task)) {
> ehca_gen_err("Can't create comp_task for cpu: %x", cpu);
> return NOTIFY_BAD;
> }
If this fails then the code will crash on CPU_UP_CANCELED. Because of
kthread_bind(cct->task,...). cct->task would be just the encoded error
number.
> @@ -730,7 +732,7 @@ int ehca_create_comp_pool(void)
>
> for_each_online_cpu(cpu) {
> task = create_comp_task(pool, cpu);
> - if (task) {
> + if (!IS_ERR(task)) {
> kthread_bind(task, cpu);
> wake_up_process(task);
> }
So you silently ignore errors and the module loads anyway?
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH -mm] ehca: avoid crash on kthread_create() failure
2006-12-21 21:22 ` Heiko Carstens
@ 2006-12-25 8:12 ` Akinobu Mita
2006-12-25 8:30 ` Akinobu Mita
2006-12-25 8:13 ` [PATCH -mm] return error on create_comp_task() failure Akinobu Mita
2006-12-25 8:14 ` [PATCH -mm] ehca: fix memleak on module unloading Akinobu Mita
2 siblings, 1 reply; 11+ messages in thread
From: Akinobu Mita @ 2006-12-25 8:12 UTC (permalink / raw)
To: Heiko Carstens; +Cc: linux-kernel, Hoang-Nam Nguyen, Christoph Raisch, akpm
On Thu, Dec 21, 2006 at 10:22:02PM +0100, Heiko Carstens wrote:
> > Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> > ===================================================================
> > --- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
> > +++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> > @@ -670,11 +670,13 @@ static int comp_pool_callback(struct not
> > {
> > unsigned int cpu = (unsigned long)hcpu;
> > struct ehca_cpu_comp_task *cct;
> > + struct task_struct *task;
> >
> > switch (action) {
> > case CPU_UP_PREPARE:
> > ehca_gen_dbg("CPU: %x (CPU_PREPARE)", cpu);
> > - if(!create_comp_task(pool, cpu)) {
> > + task = create_comp_task(pool, cpu);
> > + if (IS_ERR(task)) {
> > ehca_gen_err("Can't create comp_task for cpu: %x", cpu);
> > return NOTIFY_BAD;
> > }
>
> If this fails then the code will crash on CPU_UP_CANCELED. Because of
> kthread_bind(cct->task,...). cct->task would be just the encoded error
> number.
Subject: [PATCH -mm] ehca: avoid crash on kthread_create() failure
This patch disallows invalid task_struct pointer returned by
kthread_create() to be written to percpu data to avoid crash.
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
Cc: Christoph Raisch <raisch@de.ibm.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
drivers/infiniband/hw/ehca/ehca_irq.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
===================================================================
--- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
+++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
@@ -606,13 +606,16 @@ static int comp_task(void *__cct)
static struct task_struct *create_comp_task(struct ehca_comp_pool *pool,
int cpu)
{
+ struct task_struct *task;
struct ehca_cpu_comp_task *cct;
cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
spin_lock_init(&cct->task_lock);
INIT_LIST_HEAD(&cct->cq_list);
init_waitqueue_head(&cct->wait_queue);
- cct->task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
+ task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
+ if (!IS_ERR(task))
+ cct->task = task;
return cct->task;
}
@@ -684,8 +687,10 @@ static int comp_pool_callback(struct not
case CPU_UP_CANCELED:
ehca_gen_dbg("CPU: %x (CPU_CANCELED)", cpu);
cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
- kthread_bind(cct->task, any_online_cpu(cpu_online_map));
- destroy_comp_task(pool, cpu);
+ if (cct->task) {
+ kthread_bind(cct->task, any_online_cpu(cpu_online_map));
+ destroy_comp_task(pool, cpu);
+ }
break;
case CPU_ONLINE:
ehca_gen_dbg("CPU: %x (CPU_ONLINE)", cpu);
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH -mm] return error on create_comp_task() failure
2006-12-21 21:22 ` Heiko Carstens
2006-12-25 8:12 ` [PATCH -mm] ehca: avoid crash on kthread_create() failure Akinobu Mita
@ 2006-12-25 8:13 ` Akinobu Mita
2006-12-25 8:14 ` [PATCH -mm] ehca: fix memleak on module unloading Akinobu Mita
2 siblings, 0 replies; 11+ messages in thread
From: Akinobu Mita @ 2006-12-25 8:13 UTC (permalink / raw)
To: Heiko Carstens; +Cc: linux-kernel, Hoang-Nam Nguyen, Christoph Raisch, akpm
On Thu, Dec 21, 2006 at 10:22:02PM +0100, Heiko Carstens wrote:
> > @@ -730,7 +732,7 @@ int ehca_create_comp_pool(void)
> >
> > for_each_online_cpu(cpu) {
> > task = create_comp_task(pool, cpu);
> > - if (task) {
> > + if (!IS_ERR(task)) {
> > kthread_bind(task, cpu);
> > wake_up_process(task);
> > }
>
> So you silently ignore errors and the module loads anyway?
Subject: [PATCH -mm] return error on create_comp_task() failure
This patch frees allocated data and returns error
to avoid module loading if create_comp_task() failed.
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
Cc: Christoph Raisch <raisch@de.ibm.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
drivers/infiniband/hw/ehca/ehca_irq.c | 37 ++++++++++++++++++++++++++--------
1 file changed, 29 insertions(+), 8 deletions(-)
Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
===================================================================
--- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
+++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
@@ -716,11 +716,12 @@ static int comp_pool_callback(struct not
#endif
+#ifdef CONFIG_INFINIBAND_EHCA_SCALING
+
int ehca_create_comp_pool(void)
{
-#ifdef CONFIG_INFINIBAND_EHCA_SCALING
int cpu;
- struct task_struct *task;
+ int err;
pool = kzalloc(sizeof(struct ehca_comp_pool), GFP_KERNEL);
if (pool == NULL)
@@ -736,21 +737,41 @@ int ehca_create_comp_pool(void)
}
for_each_online_cpu(cpu) {
- task = create_comp_task(pool, cpu);
- if (!IS_ERR(task)) {
- kthread_bind(task, cpu);
- wake_up_process(task);
+ struct task_struct *task = create_comp_task(pool, cpu);
+
+ if (IS_ERR(task)) {
+ err = PTR_ERR(task);
+ goto err_comp_task;
}
+ kthread_bind(task, cpu);
+ wake_up_process(task);
}
comp_pool_callback_nb.notifier_call = comp_pool_callback;
- comp_pool_callback_nb.priority =0;
+ comp_pool_callback_nb.priority = 0;
register_cpu_notifier(&comp_pool_callback_nb);
-#endif
return 0;
+
+err_comp_task:
+ for_each_online_cpu(cpu)
+ destroy_comp_task(pool, cpu);
+
+ free_percpu(pool->cpu_comp_tasks);
+ kfree(pool);
+
+ return err;
+}
+
+#else
+
+int ehca_create_comp_pool(void)
+{
+ return 0;
}
+#endif
+
void ehca_destroy_comp_pool(void)
{
#ifdef CONFIG_INFINIBAND_EHCA_SCALING
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH -mm] ehca: fix memleak on module unloading
2006-12-21 21:22 ` Heiko Carstens
2006-12-25 8:12 ` [PATCH -mm] ehca: avoid crash on kthread_create() failure Akinobu Mita
2006-12-25 8:13 ` [PATCH -mm] return error on create_comp_task() failure Akinobu Mita
@ 2006-12-25 8:14 ` Akinobu Mita
2 siblings, 0 replies; 11+ messages in thread
From: Akinobu Mita @ 2006-12-25 8:14 UTC (permalink / raw)
To: Heiko Carstens; +Cc: linux-kernel, Hoang-Nam Nguyen, Christoph Raisch, akpm
Subject: [PATCH] ehca: fix memleak on module unloading
percpu data is not freed on module unloading.
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
Cc: Christoph Raisch <raisch@de.ibm.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
drivers/infiniband/hw/ehca/ehca_irq.c | 2 ++
1 file changed, 2 insertions(+)
Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
===================================================================
--- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
+++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
@@ -757,6 +757,8 @@ void ehca_destroy_comp_pool(void)
if (cpu_online(i))
destroy_comp_task(pool, i);
}
+ free_percpu(pool->cpu_comp_tasks);
+ kfree(pool);
#endif
return;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure
2006-12-25 8:12 ` [PATCH -mm] ehca: avoid crash on kthread_create() failure Akinobu Mita
@ 2006-12-25 8:30 ` Akinobu Mita
2006-12-25 8:55 ` Muli Ben-Yehuda
0 siblings, 1 reply; 11+ messages in thread
From: Akinobu Mita @ 2006-12-25 8:30 UTC (permalink / raw)
To: Heiko Carstens, linux-kernel, Hoang-Nam Nguyen, Christoph Raisch,
akpm
On Mon, Dec 25, 2006 at 05:12:57PM +0900, Akinobu Mita wrote:
> Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> ===================================================================
> --- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
> +++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> @@ -606,13 +606,16 @@ static int comp_task(void *__cct)
> static struct task_struct *create_comp_task(struct ehca_comp_pool *pool,
> int cpu)
> {
> + struct task_struct *task;
> struct ehca_cpu_comp_task *cct;
>
> cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
> spin_lock_init(&cct->task_lock);
> INIT_LIST_HEAD(&cct->cq_list);
> init_waitqueue_head(&cct->wait_queue);
> - cct->task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> + task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> + if (!IS_ERR(task))
> + cct->task = task;
>
> return cct->task;
> }
This patch is wrong. It changes to make create_comp_task() return NULL
on failure.
(BTW, all these ehca fixes are not compile tested due to luck of
cross comile environment)
Subject: [PATCH -mm] ehca: avoid crash on kthread_create() failure (v2)
This patch disallows invalid task_struct pointer returned by
kthread_create() to be written to percpu data to avoid crash.
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
Cc: Christoph Raisch <raisch@de.ibm.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
drivers/infiniband/hw/ehca/ehca_irq.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
===================================================================
--- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
+++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
@@ -606,15 +606,18 @@ static int comp_task(void *__cct)
static struct task_struct *create_comp_task(struct ehca_comp_pool *pool,
int cpu)
{
+ struct task_struct *task;
struct ehca_cpu_comp_task *cct;
cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
spin_lock_init(&cct->task_lock);
INIT_LIST_HEAD(&cct->cq_list);
init_waitqueue_head(&cct->wait_queue);
- cct->task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
+ task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
+ if (!IS_ERR(task))
+ cct->task = task;
- return cct->task;
+ return task;
}
static void destroy_comp_task(struct ehca_comp_pool *pool,
@@ -684,8 +687,10 @@ static int comp_pool_callback(struct not
case CPU_UP_CANCELED:
ehca_gen_dbg("CPU: %x (CPU_CANCELED)", cpu);
cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
- kthread_bind(cct->task, any_online_cpu(cpu_online_map));
- destroy_comp_task(pool, cpu);
+ if (cct->task) {
+ kthread_bind(cct->task, any_online_cpu(cpu_online_map));
+ destroy_comp_task(pool, cpu);
+ }
break;
case CPU_ONLINE:
ehca_gen_dbg("CPU: %x (CPU_ONLINE)", cpu);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure
2006-12-25 8:30 ` Akinobu Mita
@ 2006-12-25 8:55 ` Muli Ben-Yehuda
2006-12-25 9:35 ` Akinobu Mita
0 siblings, 1 reply; 11+ messages in thread
From: Muli Ben-Yehuda @ 2006-12-25 8:55 UTC (permalink / raw)
To: Akinobu Mita
Cc: Heiko Carstens, linux-kernel, Hoang-Nam Nguyen, Christoph Raisch,
akpm
On Mon, Dec 25, 2006 at 05:30:23PM +0900, Akinobu Mita wrote:
> On Mon, Dec 25, 2006 at 05:12:57PM +0900, Akinobu Mita wrote:
> > Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> > ===================================================================
> > --- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
> > +++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> > @@ -606,13 +606,16 @@ static int comp_task(void *__cct)
> > static struct task_struct *create_comp_task(struct ehca_comp_pool *pool,
> > int cpu)
> > {
> > + struct task_struct *task;
> > struct ehca_cpu_comp_task *cct;
> >
> > cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
> > spin_lock_init(&cct->task_lock);
> > INIT_LIST_HEAD(&cct->cq_list);
> > init_waitqueue_head(&cct->wait_queue);
> > - cct->task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> > + task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> > + if (!IS_ERR(task))
> > + cct->task = task;
> >
> > return cct->task;
> > }
>
> This patch is wrong. It changes to make create_comp_task() return NULL
> on failure.
>
> (BTW, all these ehca fixes are not compile tested due to luck of
> cross comile environment)
>
> Subject: [PATCH -mm] ehca: avoid crash on kthread_create() failure (v2)
>
> This patch disallows invalid task_struct pointer returned by
> kthread_create() to be written to percpu data to avoid crash.
>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
> Cc: Christoph Raisch <raisch@de.ibm.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>
> ---
> drivers/infiniband/hw/ehca/ehca_irq.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> ===================================================================
> --- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
> +++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> @@ -606,15 +606,18 @@ static int comp_task(void *__cct)
> static struct task_struct *create_comp_task(struct ehca_comp_pool *pool,
> int cpu)
> {
> + struct task_struct *task;
> struct ehca_cpu_comp_task *cct;
>
> cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
> spin_lock_init(&cct->task_lock);
> INIT_LIST_HEAD(&cct->cq_list);
> init_waitqueue_head(&cct->wait_queue);
> - cct->task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> + task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> + if (!IS_ERR(task))
> + cct->task = task;
>
> - return cct->task;
> + return task;
> }
>
> static void destroy_comp_task(struct ehca_comp_pool *pool,
> @@ -684,8 +687,10 @@ static int comp_pool_callback(struct not
> case CPU_UP_CANCELED:
> ehca_gen_dbg("CPU: %x (CPU_CANCELED)", cpu);
> cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
> - kthread_bind(cct->task, any_online_cpu(cpu_online_map));
> - destroy_comp_task(pool, cpu);
> + if (cct->task) {
> + kthread_bind(cct->task, any_online_cpu(cpu_online_map));
> + destroy_comp_task(pool, cpu);
> + }
This is correct because cct is allocated via alloc_percpu, which in
turn calls kzalloc, which means cct->task is NULL by default, but it's
a little too obscure for me. How about making it explicit?
task = kthread_create(...)
if (!IS_ERR(task))
cct->task = task;
else
cct->task = NULL;
return cct->task;
Cheers,
Muli
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure
2006-12-25 8:55 ` Muli Ben-Yehuda
@ 2006-12-25 9:35 ` Akinobu Mita
2006-12-25 9:41 ` Muli Ben-Yehuda
0 siblings, 1 reply; 11+ messages in thread
From: Akinobu Mita @ 2006-12-25 9:35 UTC (permalink / raw)
To: Muli Ben-Yehuda
Cc: Heiko Carstens, linux-kernel, Hoang-Nam Nguyen, Christoph Raisch,
akpm
On Mon, Dec 25, 2006 at 10:55:51AM +0200, Muli Ben-Yehuda wrote:
> This is correct because cct is allocated via alloc_percpu, which in
> turn calls kzalloc, which means cct->task is NULL by default, but it's
> a little too obscure for me. How about making it explicit?
>
> task = kthread_create(...)
> if (!IS_ERR(task))
> cct->task = task;
> else
> cct->task = NULL;
>
> return cct->task;
Subject: [PATCH -mm] ehca: avoid crash on kthread_create() failure (v3)
This patch disallows invalid task_struct pointer returned by
kthread_create() to be written to percpu data to avoid crash.
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
Cc: Christoph Raisch <raisch@de.ibm.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
drivers/infiniband/hw/ehca/ehca_irq.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
===================================================================
--- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
+++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
@@ -606,15 +606,20 @@ static int comp_task(void *__cct)
static struct task_struct *create_comp_task(struct ehca_comp_pool *pool,
int cpu)
{
+ struct task_struct *task;
struct ehca_cpu_comp_task *cct;
cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
spin_lock_init(&cct->task_lock);
INIT_LIST_HEAD(&cct->cq_list);
init_waitqueue_head(&cct->wait_queue);
- cct->task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
+ task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
+ if (!IS_ERR(task))
+ cct->task = task;
+ else
+ cct->task = NULL;
- return cct->task;
+ return task;
}
static void destroy_comp_task(struct ehca_comp_pool *pool,
@@ -684,8 +689,10 @@ static int comp_pool_callback(struct not
case CPU_UP_CANCELED:
ehca_gen_dbg("CPU: %x (CPU_CANCELED)", cpu);
cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
- kthread_bind(cct->task, any_online_cpu(cpu_online_map));
- destroy_comp_task(pool, cpu);
+ if (cct->task) {
+ kthread_bind(cct->task, any_online_cpu(cpu_online_map));
+ destroy_comp_task(pool, cpu);
+ }
break;
case CPU_ONLINE:
ehca_gen_dbg("CPU: %x (CPU_ONLINE)", cpu);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure
2006-12-25 9:35 ` Akinobu Mita
@ 2006-12-25 9:41 ` Muli Ben-Yehuda
2006-12-25 9:58 ` Akinobu Mita
0 siblings, 1 reply; 11+ messages in thread
From: Muli Ben-Yehuda @ 2006-12-25 9:41 UTC (permalink / raw)
To: Akinobu Mita
Cc: Heiko Carstens, linux-kernel, Hoang-Nam Nguyen, Christoph Raisch,
akpm
On Mon, Dec 25, 2006 at 06:35:57PM +0900, Akinobu Mita wrote:
> On Mon, Dec 25, 2006 at 10:55:51AM +0200, Muli Ben-Yehuda wrote:
> > This is correct because cct is allocated via alloc_percpu, which in
> > turn calls kzalloc, which means cct->task is NULL by default, but it's
> > a little too obscure for me. How about making it explicit?
> >
> > task = kthread_create(...)
> > if (!IS_ERR(task))
> > cct->task = task;
> > else
> > cct->task = NULL;
> >
> > return cct->task;
>
> Subject: [PATCH -mm] ehca: avoid crash on kthread_create() failure (v3)
>
> This patch disallows invalid task_struct pointer returned by
> kthread_create() to be written to percpu data to avoid crash.
>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
> Cc: Christoph Raisch <raisch@de.ibm.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>
> ---
> drivers/infiniband/hw/ehca/ehca_irq.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> ===================================================================
> --- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
> +++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> @@ -606,15 +606,20 @@ static int comp_task(void *__cct)
> static struct task_struct *create_comp_task(struct ehca_comp_pool *pool,
> int cpu)
> {
> + struct task_struct *task;
> struct ehca_cpu_comp_task *cct;
>
> cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
> spin_lock_init(&cct->task_lock);
> INIT_LIST_HEAD(&cct->cq_list);
> init_waitqueue_head(&cct->wait_queue);
> - cct->task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> + task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> + if (!IS_ERR(task))
> + cct->task = task;
> + else
> + cct->task = NULL;
>
> - return cct->task;
> + return task;
This should be return cct->task, since we later test the return value
of create_comp_task against NULL, e.g., in comp_poll_callback and
ehca_create_comp_pool.
Cheers,
Muli
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure
2006-12-25 9:41 ` Muli Ben-Yehuda
@ 2006-12-25 9:58 ` Akinobu Mita
0 siblings, 0 replies; 11+ messages in thread
From: Akinobu Mita @ 2006-12-25 9:58 UTC (permalink / raw)
To: Muli Ben-Yehuda
Cc: Heiko Carstens, linux-kernel, Hoang-Nam Nguyen, Christoph Raisch,
akpm
On Mon, Dec 25, 2006 at 11:41:32AM +0200, Muli Ben-Yehuda wrote:
> On Mon, Dec 25, 2006 at 06:35:57PM +0900, Akinobu Mita wrote:
> > On Mon, Dec 25, 2006 at 10:55:51AM +0200, Muli Ben-Yehuda wrote:
> > > This is correct because cct is allocated via alloc_percpu, which in
> > > turn calls kzalloc, which means cct->task is NULL by default, but it's
> > > a little too obscure for me. How about making it explicit?
> > >
> > > task = kthread_create(...)
> > > if (!IS_ERR(task))
> > > cct->task = task;
> > > else
> > > cct->task = NULL;
> > >
> > > return cct->task;
> >
> > Subject: [PATCH -mm] ehca: avoid crash on kthread_create() failure (v3)
> >
> > This patch disallows invalid task_struct pointer returned by
> > kthread_create() to be written to percpu data to avoid crash.
> >
> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
> > Cc: Christoph Raisch <raisch@de.ibm.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >
> > ---
> > drivers/infiniband/hw/ehca/ehca_irq.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > Index: 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> > ===================================================================
> > --- 2.6-mm.orig/drivers/infiniband/hw/ehca/ehca_irq.c
> > +++ 2.6-mm/drivers/infiniband/hw/ehca/ehca_irq.c
> > @@ -606,15 +606,20 @@ static int comp_task(void *__cct)
> > static struct task_struct *create_comp_task(struct ehca_comp_pool *pool,
> > int cpu)
> > {
> > + struct task_struct *task;
> > struct ehca_cpu_comp_task *cct;
> >
> > cct = per_cpu_ptr(pool->cpu_comp_tasks, cpu);
> > spin_lock_init(&cct->task_lock);
> > INIT_LIST_HEAD(&cct->cq_list);
> > init_waitqueue_head(&cct->wait_queue);
> > - cct->task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> > + task = kthread_create(comp_task, cct, "ehca_comp/%d", cpu);
> > + if (!IS_ERR(task))
> > + cct->task = task;
> > + else
> > + cct->task = NULL;
> >
> > - return cct->task;
> > + return task;
>
> This should be return cct->task, since we later test the return value
> of create_comp_task against NULL, e.g., in comp_poll_callback and
> ehca_create_comp_pool.
Yeah, I already sent the patch:
http://lkml.org/lkml/2006/12/19/56
Maybe I should reorganize these ehca fixes later.
(make ehca_create_task() return NULL on failure)
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2006-12-25 9:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-12-19 8:42 [PATCH] ehca: fix kthread_create() error check Akinobu Mita
2006-12-19 9:32 ` Hoang-Nam Nguyen
2006-12-21 21:22 ` Heiko Carstens
2006-12-25 8:12 ` [PATCH -mm] ehca: avoid crash on kthread_create() failure Akinobu Mita
2006-12-25 8:30 ` Akinobu Mita
2006-12-25 8:55 ` Muli Ben-Yehuda
2006-12-25 9:35 ` Akinobu Mita
2006-12-25 9:41 ` Muli Ben-Yehuda
2006-12-25 9:58 ` Akinobu Mita
2006-12-25 8:13 ` [PATCH -mm] return error on create_comp_task() failure Akinobu Mita
2006-12-25 8:14 ` [PATCH -mm] ehca: fix memleak on module unloading Akinobu Mita
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.