[PATCH] ehca: fix kthread_create() error check

[PATCH] ehca: fix kthread_create() error check

[Akinobu Mita]

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);
 		}

Re: [PATCH] ehca: fix kthread_create() error check

[Hoang-Nam Nguyen]

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

Re: [PATCH] ehca: fix kthread_create() error check

[Heiko Carstens]

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

[PATCH -mm] ehca: avoid crash on kthread_create() failure

[Akinobu Mita]

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

Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure

[Akinobu Mita]

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

Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure

[Muli Ben-Yehuda]

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

Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure

[Akinobu Mita]

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

Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure

[Muli Ben-Yehuda]

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

Re: [PATCH -mm] ehca: avoid crash on kthread_create() failure

[Akinobu Mita]

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)

[PATCH -mm] return error on create_comp_task() failure

[Akinobu Mita]

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

[PATCH -mm] ehca: fix memleak on module unloading

[Akinobu Mita]

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;