[KJ] audit return code handling for kernel_thread [1/11]

[KJ] audit return code handling for kernel_thread [1/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/macintosh/adb.c        |    4 ++++
 1 file changed, 4 insertions(+)
--- a/drivers/macintosh/adb.c
+++ b/drivers/macintosh/adb.c
@@ -270,6 +270,10 @@ static void
 __adb_probe_task(void *data)
 {
 	adb_probe_task_pid = kernel_thread(adb_probe_task, NULL, SIGCHLD | CLONE_KERNEL);
+	if (adb_probe_task_pid < 0) {
+		printk(KERN_WARNING "Could not start adb probe task\n");
+		adb_probe_task_pid = 0;
+	}
 }
 
 static DECLARE_WORK(adb_reset_work, __adb_probe_task, NULL);
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [1/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/macintosh/adb.c        |    4 ++++
 1 file changed, 4 insertions(+)
--- a/drivers/macintosh/adb.c
+++ b/drivers/macintosh/adb.c
@@ -270,6 +270,10 @@ static void
 __adb_probe_task(void *data)
 {
 	adb_probe_task_pid = kernel_thread(adb_probe_task, NULL, SIGCHLD | CLONE_KERNEL);
+	if (adb_probe_task_pid < 0) {
+		printk(KERN_WARNING "Could not start adb probe task\n");
+		adb_probe_task_pid = 0;
+	}
 }
 
 static DECLARE_WORK(adb_reset_work, __adb_probe_task, NULL);

[KJ] audit return code handling for kernel_thread [2/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 arch/s390/mm/cmm.c           |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -161,7 +161,10 @@ cmm_thread(void *dummy)
 static void
 cmm_start_thread(void)
 {
-	kernel_thread(cmm_thread, NULL, 0);
+	if (kernel_thread(cmm_thread, NULL, 0) < 0) {
+		printk(KERN_WARNING "Could not start cmm thread\n");
+		clear_bit(0,&cmm_thread_active);
+	}
 }
 
 static void
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [2/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 arch/s390/mm/cmm.c           |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -161,7 +161,10 @@ cmm_thread(void *dummy)
 static void
 cmm_start_thread(void)
 {
-	kernel_thread(cmm_thread, NULL, 0);
+	if (kernel_thread(cmm_thread, NULL, 0) < 0) {
+		printk(KERN_WARNING "Could not start cmm thread\n");
+		clear_bit(0,&cmm_thread_active);
+	}
 }
 
 static void

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Russell King]

On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
>     Problems seemed to fall into 3 main categories:
>     
>     1) callers of kernel_thread were inconsistent about meaning of a zero return
>     code.  Some callers considered a zero return code to mean success, others took
>     it to mean failure.  a zero return code, while not actually possible in the
>     current implementation, should be considered a success (pid 0 is/should be
>     valid). fixed all callers to treat zero return as success
>     
>     2) caller of kernel_thread saved return code of kernel_thread for later use
>     without ever checking its value.  Callers who did this tended to assume a
>     non-zero return was success, and would often wait for a completion queue to be
>     woken up, implying that an error (negative return code) from kernel_thread could
>     lead to deadlock.  Repaired by checking return code at call time, and setting
>     saved return code to zero in the event of an error.

This is inconsistent with your assertion that pid 0 "is/should be valid"
above.  If you want '0' to mean "not valid" then it's not a valid return
value from kernel_thread() (and arguably that's true, since pid 0 is
permanently allocated to the idle thread.)

I don't particularly care whether you decide to that returning pid 0 from
kernel_thread is valid or not, just that your two points above are at least
consistent with each other.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Russell King]

On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
>     Problems seemed to fall into 3 main categories:
>     
>     1) callers of kernel_thread were inconsistent about meaning of a zero return
>     code.  Some callers considered a zero return code to mean success, others took
>     it to mean failure.  a zero return code, while not actually possible in the
>     current implementation, should be considered a success (pid 0 is/should be
>     valid). fixed all callers to treat zero return as success
>     
>     2) caller of kernel_thread saved return code of kernel_thread for later use
>     without ever checking its value.  Callers who did this tended to assume a
>     non-zero return was success, and would often wait for a completion queue to be
>     woken up, implying that an error (negative return code) from kernel_thread could
>     lead to deadlock.  Repaired by checking return code at call time, and setting
>     saved return code to zero in the event of an error.

This is inconsistent with your assertion that pid 0 "is/should be valid"
above.  If you want '0' to mean "not valid" then it's not a valid return
value from kernel_thread() (and arguably that's true, since pid 0 is
permanently allocated to the idle thread.)

I don't particularly care whether you decide to that returning pid 0 from
kernel_thread is valid or not, just that your two points above are at least
consistent with each other.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> >     Problems seemed to fall into 3 main categories:
> >     
> >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> >     code.  Some callers considered a zero return code to mean success, others took
> >     it to mean failure.  a zero return code, while not actually possible in the
> >     current implementation, should be considered a success (pid 0 is/should be
> >     valid). fixed all callers to treat zero return as success
> >     
> >     2) caller of kernel_thread saved return code of kernel_thread for later use
> >     without ever checking its value.  Callers who did this tended to assume a
> >     non-zero return was success, and would often wait for a completion queue to be
> >     woken up, implying that an error (negative return code) from kernel_thread could
> >     lead to deadlock.  Repaired by checking return code at call time, and setting
> >     saved return code to zero in the event of an error.
> 
> This is inconsistent with your assertion that pid 0 "is/should be valid"
> above.  If you want '0' to mean "not valid" then it's not a valid return
> value from kernel_thread() (and arguably that's true, since pid 0 is
> permanently allocated to the idle thread.)
> 
I think you misread.  I want a return code of zero to be valid (and imply
success).  However, kernel_thread returns an int (not an unsigned int), and
there are/were callers who assumed that _any_ non-zero return values were
success, including negative return values, which indicate a failure in
kernel_thread.

> I don't particularly care whether you decide to that returning pid 0 from
> kernel_thread is valid or not, just that your two points above are at least
> consistent with each other.
> 
I should have been more clear above, point two is meant to indicate that there
were callers of kernel_thread which assume a negative return code from
kernel_thread meant success.  That is what I fixed.

Regards
Neil

> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:  2.6 Serial core
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> >     Problems seemed to fall into 3 main categories:
> >     
> >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> >     code.  Some callers considered a zero return code to mean success, others took
> >     it to mean failure.  a zero return code, while not actually possible in the
> >     current implementation, should be considered a success (pid 0 is/should be
> >     valid). fixed all callers to treat zero return as success
> >     
> >     2) caller of kernel_thread saved return code of kernel_thread for later use
> >     without ever checking its value.  Callers who did this tended to assume a
> >     non-zero return was success, and would often wait for a completion queue to be
> >     woken up, implying that an error (negative return code) from kernel_thread could
> >     lead to deadlock.  Repaired by checking return code at call time, and setting
> >     saved return code to zero in the event of an error.
> 
> This is inconsistent with your assertion that pid 0 "is/should be valid"
> above.  If you want '0' to mean "not valid" then it's not a valid return
> value from kernel_thread() (and arguably that's true, since pid 0 is
> permanently allocated to the idle thread.)
> 
I think you misread.  I want a return code of zero to be valid (and imply
success).  However, kernel_thread returns an int (not an unsigned int), and
there are/were callers who assumed that _any_ non-zero return values were
success, including negative return values, which indicate a failure in
kernel_thread.

> I don't particularly care whether you decide to that returning pid 0 from
> kernel_thread is valid or not, just that your two points above are at least
> consistent with each other.
> 
I should have been more clear above, point two is meant to indicate that there
were callers of kernel_thread which assume a negative return code from
kernel_thread meant success.  That is what I fixed.

Regards
Neil

> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:  2.6 Serial core

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> >     Problems seemed to fall into 3 main categories:
> >     
> >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> >     code.  Some callers considered a zero return code to mean success, others took
> >     it to mean failure.  a zero return code, while not actually possible in the
> >     current implementation, should be considered a success (pid 0 is/should be
> >     valid). fixed all callers to treat zero return as success
> >     
> >     2) caller of kernel_thread saved return code of kernel_thread for later use
> >     without ever checking its value.  Callers who did this tended to assume a
> >     non-zero return was success, and would often wait for a completion queue to be
> >     woken up, implying that an error (negative return code) from kernel_thread could
> >     lead to deadlock.  Repaired by checking return code at call time, and setting
> >     saved return code to zero in the event of an error.
> 
> This is inconsistent with your assertion that pid 0 "is/should be valid"
> above.  If you want '0' to mean "not valid" then it's not a valid return
> value from kernel_thread() (and arguably that's true, since pid 0 is
> permanently allocated to the idle thread.)
> 
No its, not, but I can see how my comments might be ambiguous. I want zero to be
a valid return code, since we never actually return zero, but we certainly could
if we wanted to.  Note that kernel_thread returns an int (not an unsigned int),
and as such assuming that a non-zero return code implies success ignores the
fact that kernel_thread can return a negative value, which indicates failure.
This is what I found, and what my patch fixes.

> I don't particularly care whether you decide to that returning pid 0 from
> kernel_thread is valid or not, just that your two points above are at least
> consistent with each other.
> 
My comments in (2) should be made more clear by changing "assume a non-zero
return was success" to "assume a negative return was success".  This is what my
patch fixes.

Thanks & Regards
Neil

> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:  2.6 Serial core
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> >     Problems seemed to fall into 3 main categories:
> >     
> >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> >     code.  Some callers considered a zero return code to mean success, others took
> >     it to mean failure.  a zero return code, while not actually possible in the
> >     current implementation, should be considered a success (pid 0 is/should be
> >     valid). fixed all callers to treat zero return as success
> >     
> >     2) caller of kernel_thread saved return code of kernel_thread for later use
> >     without ever checking its value.  Callers who did this tended to assume a
> >     non-zero return was success, and would often wait for a completion queue to be
> >     woken up, implying that an error (negative return code) from kernel_thread could
> >     lead to deadlock.  Repaired by checking return code at call time, and setting
> >     saved return code to zero in the event of an error.
> 
> This is inconsistent with your assertion that pid 0 "is/should be valid"
> above.  If you want '0' to mean "not valid" then it's not a valid return
> value from kernel_thread() (and arguably that's true, since pid 0 is
> permanently allocated to the idle thread.)
> 
No its, not, but I can see how my comments might be ambiguous. I want zero to be
a valid return code, since we never actually return zero, but we certainly could
if we wanted to.  Note that kernel_thread returns an int (not an unsigned int),
and as such assuming that a non-zero return code implies success ignores the
fact that kernel_thread can return a negative value, which indicates failure.
This is what I found, and what my patch fixes.

> I don't particularly care whether you decide to that returning pid 0 from
> kernel_thread is valid or not, just that your two points above are at least
> consistent with each other.
> 
My comments in (2) should be made more clear by changing "assume a non-zero
return was success" to "assume a negative return was success".  This is what my
patch fixes.

Thanks & Regards
Neil

> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:  2.6 Serial core

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 09:14:19AM -0400, Neil Horman wrote:
> On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> > On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> > >     Problems seemed to fall into 3 main categories:
> > >     
> > >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> > >     code.  Some callers considered a zero return code to mean success, others took
> > >     it to mean failure.  a zero return code, while not actually possible in the
> > >     current implementation, should be considered a success (pid 0 is/should be
> > >     valid). fixed all callers to treat zero return as success
> > >     
> > >     2) caller of kernel_thread saved return code of kernel_thread for later use
> > >     without ever checking its value.  Callers who did this tended to assume a
> > >     non-zero return was success, and would often wait for a completion queue to be
> > >     woken up, implying that an error (negative return code) from kernel_thread could
> > >     lead to deadlock.  Repaired by checking return code at call time, and setting
> > >     saved return code to zero in the event of an error.
> > 
> > This is inconsistent with your assertion that pid 0 "is/should be valid"
> > above.  If you want '0' to mean "not valid" then it's not a valid return
> > value from kernel_thread() (and arguably that's true, since pid 0 is
> > permanently allocated to the idle thread.)
> > 
> No its, not, but I can see how my comments might be ambiguous. I want zero to be
> a valid return code, since we never actually return zero, but we certainly could
> if we wanted to.  Note that kernel_thread returns an int (not an unsigned int),
> and as such assuming that a non-zero return code implies success ignores the
> fact that kernel_thread can return a negative value, which indicates failure.
> This is what I found, and what my patch fixes.
> 
> > I don't particularly care whether you decide to that returning pid 0 from
> > kernel_thread is valid or not, just that your two points above are at least
> > consistent with each other.
> > 
> My comments in (2) should be made more clear by changing "assume a non-zero
> return was success" to "assume a negative return was success".  This is what my
> patch fixes.
> 
> Thanks & Regards
> Neil
> 


P.S. - Sorry, Russell, et al.  for the double post.  My network link went out and I accendentally
sent two replies to your note

Neil

> > -- 
> > Russell King
> >  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
> >  maintainer of:  2.6 Serial core
> -
> 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/
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 09:14:19AM -0400, Neil Horman wrote:
> On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> > On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> > >     Problems seemed to fall into 3 main categories:
> > >     
> > >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> > >     code.  Some callers considered a zero return code to mean success, others took
> > >     it to mean failure.  a zero return code, while not actually possible in the
> > >     current implementation, should be considered a success (pid 0 is/should be
> > >     valid). fixed all callers to treat zero return as success
> > >     
> > >     2) caller of kernel_thread saved return code of kernel_thread for later use
> > >     without ever checking its value.  Callers who did this tended to assume a
> > >     non-zero return was success, and would often wait for a completion queue to be
> > >     woken up, implying that an error (negative return code) from kernel_thread could
> > >     lead to deadlock.  Repaired by checking return code at call time, and setting
> > >     saved return code to zero in the event of an error.
> > 
> > This is inconsistent with your assertion that pid 0 "is/should be valid"
> > above.  If you want '0' to mean "not valid" then it's not a valid return
> > value from kernel_thread() (and arguably that's true, since pid 0 is
> > permanently allocated to the idle thread.)
> > 
> No its, not, but I can see how my comments might be ambiguous. I want zero to be
> a valid return code, since we never actually return zero, but we certainly could
> if we wanted to.  Note that kernel_thread returns an int (not an unsigned int),
> and as such assuming that a non-zero return code implies success ignores the
> fact that kernel_thread can return a negative value, which indicates failure.
> This is what I found, and what my patch fixes.
> 
> > I don't particularly care whether you decide to that returning pid 0 from
> > kernel_thread is valid or not, just that your two points above are at least
> > consistent with each other.
> > 
> My comments in (2) should be made more clear by changing "assume a non-zero
> return was success" to "assume a negative return was success".  This is what my
> patch fixes.
> 
> Thanks & Regards
> Neil
> 


P.S. - Sorry, Russell, et al.  for the double post.  My network link went out and I accendentally
sent two replies to your note

Neil

> > -- 
> > Russell King
> >  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
> >  maintainer of:  2.6 Serial core
> -
> 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/

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Russell King]

On Sat, Jul 29, 2006 at 09:14:19AM -0400, Neil Horman wrote:
> On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> > On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> > >     Problems seemed to fall into 3 main categories:
> > >     
> > >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> > >     code.  Some callers considered a zero return code to mean success, others took
> > >     it to mean failure.  a zero return code, while not actually possible in the
> > >     current implementation, should be considered a success (pid 0 is/should be
> > >     valid). fixed all callers to treat zero return as success
> > >     
> > >     2) caller of kernel_thread saved return code of kernel_thread for later use
> > >     without ever checking its value.  Callers who did this tended to assume a
> > >     non-zero return was success, and would often wait for a completion queue to be
> > >     woken up, implying that an error (negative return code) from kernel_thread could
> > >     lead to deadlock.  Repaired by checking return code at call time, and setting
> > >     saved return code to zero in the event of an error.
> > 
> > This is inconsistent with your assertion that pid 0 "is/should be valid"
> > above.  If you want '0' to mean "not valid" then it's not a valid return
> > value from kernel_thread() (and arguably that's true, since pid 0 is
> > permanently allocated to the idle thread.)
> > 
> No its, not, but I can see how my comments might be ambiguous. I want zero to be
> a valid return code, since we never actually return zero, but we certainly could
> if we wanted to.  Note that kernel_thread returns an int (not an unsigned int),
> and as such assuming that a non-zero return code implies success ignores the
> fact that kernel_thread can return a negative value, which indicates failure.
> This is what I found, and what my patch fixes.
> 
> > I don't particularly care whether you decide to that returning pid 0 from
> > kernel_thread is valid or not, just that your two points above are at least
> > consistent with each other.
> > 
> My comments in (2) should be made more clear by changing "assume a non-zero
> return was success" to "assume a negative return was success".  This is what my
> patch fixes.

In the first point, you say that you want return of zero to mean success.
In the second point, you use zero to mark an error event.  That's the
inconsistency I'm referring to.

So, if we _did_ return zero from kernel_thread, and we had your fix as in
(2), you'd store zero into the saved return code, which would then be
interpreted in later code as an error.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Russell King]

On Sat, Jul 29, 2006 at 09:14:19AM -0400, Neil Horman wrote:
> On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> > On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> > >     Problems seemed to fall into 3 main categories:
> > >     
> > >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> > >     code.  Some callers considered a zero return code to mean success, others took
> > >     it to mean failure.  a zero return code, while not actually possible in the
> > >     current implementation, should be considered a success (pid 0 is/should be
> > >     valid). fixed all callers to treat zero return as success
> > >     
> > >     2) caller of kernel_thread saved return code of kernel_thread for later use
> > >     without ever checking its value.  Callers who did this tended to assume a
> > >     non-zero return was success, and would often wait for a completion queue to be
> > >     woken up, implying that an error (negative return code) from kernel_thread could
> > >     lead to deadlock.  Repaired by checking return code at call time, and setting
> > >     saved return code to zero in the event of an error.
> > 
> > This is inconsistent with your assertion that pid 0 "is/should be valid"
> > above.  If you want '0' to mean "not valid" then it's not a valid return
> > value from kernel_thread() (and arguably that's true, since pid 0 is
> > permanently allocated to the idle thread.)
> > 
> No its, not, but I can see how my comments might be ambiguous. I want zero to be
> a valid return code, since we never actually return zero, but we certainly could
> if we wanted to.  Note that kernel_thread returns an int (not an unsigned int),
> and as such assuming that a non-zero return code implies success ignores the
> fact that kernel_thread can return a negative value, which indicates failure.
> This is what I found, and what my patch fixes.
> 
> > I don't particularly care whether you decide to that returning pid 0 from
> > kernel_thread is valid or not, just that your two points above are at least
> > consistent with each other.
> > 
> My comments in (2) should be made more clear by changing "assume a non-zero
> return was success" to "assume a negative return was success".  This is what my
> patch fixes.

In the first point, you say that you want return of zero to mean success.
In the second point, you use zero to mark an error event.  That's the
inconsistency I'm referring to.

So, if we _did_ return zero from kernel_thread, and we had your fix as in
(2), you'd store zero into the saved return code, which would then be
interpreted in later code as an error.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 03:50:04PM +0100, Russell King wrote:
> On Sat, Jul 29, 2006 at 09:14:19AM -0400, Neil Horman wrote:
> > On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> > > On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > > > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> > > >     Problems seemed to fall into 3 main categories:
> > > >     
> > > >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> > > >     code.  Some callers considered a zero return code to mean success, others took
> > > >     it to mean failure.  a zero return code, while not actually possible in the
> > > >     current implementation, should be considered a success (pid 0 is/should be
> > > >     valid). fixed all callers to treat zero return as success
> > > >     
> > > >     2) caller of kernel_thread saved return code of kernel_thread for later use
> > > >     without ever checking its value.  Callers who did this tended to assume a
> > > >     non-zero return was success, and would often wait for a completion queue to be
> > > >     woken up, implying that an error (negative return code) from kernel_thread could
> > > >     lead to deadlock.  Repaired by checking return code at call time, and setting
> > > >     saved return code to zero in the event of an error.
> > > 
> > > This is inconsistent with your assertion that pid 0 "is/should be valid"
> > > above.  If you want '0' to mean "not valid" then it's not a valid return
> > > value from kernel_thread() (and arguably that's true, since pid 0 is
> > > permanently allocated to the idle thread.)
> > > 
> > No its, not, but I can see how my comments might be ambiguous. I want zero to be
> > a valid return code, since we never actually return zero, but we certainly could
> > if we wanted to.  Note that kernel_thread returns an int (not an unsigned int),
> > and as such assuming that a non-zero return code implies success ignores the
> > fact that kernel_thread can return a negative value, which indicates failure.
> > This is what I found, and what my patch fixes.
> > 
> > > I don't particularly care whether you decide to that returning pid 0 from
> > > kernel_thread is valid or not, just that your two points above are at least
> > > consistent with each other.
> > > 
> > My comments in (2) should be made more clear by changing "assume a non-zero
> > return was success" to "assume a negative return was success".  This is what my
> > patch fixes.
> 
> In the first point, you say that you want return of zero to mean success.
> In the second point, you use zero to mark an error event.  That's the
> inconsistency I'm referring to.
> 
> So, if we _did_ return zero from kernel_thread, and we had your fix as in
> (2), you'd store zero into the saved return code, which would then be
> interpreted in later code as an error.
> 
Ahh, I see what your saying now.  The later check of the stored return code
should specifically check for negative, rather than non-zero return codes,
instead of just assigning zero to the stored result to avoid the later check
going down the wrong return path, good point.  I'll fix that up early next week,
with the other items on my todo list.

Thanks & Regards
Neil

> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:  2.6 Serial core
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [2/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 03:50:04PM +0100, Russell King wrote:
> On Sat, Jul 29, 2006 at 09:14:19AM -0400, Neil Horman wrote:
> > On Sat, Jul 29, 2006 at 10:37:04AM +0100, Russell King wrote:
> > > On Fri, Jul 28, 2006 at 04:07:13PM -0400, nhorman@tuxdriver.com wrote:
> > > > Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
> > > >     Problems seemed to fall into 3 main categories:
> > > >     
> > > >     1) callers of kernel_thread were inconsistent about meaning of a zero return
> > > >     code.  Some callers considered a zero return code to mean success, others took
> > > >     it to mean failure.  a zero return code, while not actually possible in the
> > > >     current implementation, should be considered a success (pid 0 is/should be
> > > >     valid). fixed all callers to treat zero return as success
> > > >     
> > > >     2) caller of kernel_thread saved return code of kernel_thread for later use
> > > >     without ever checking its value.  Callers who did this tended to assume a
> > > >     non-zero return was success, and would often wait for a completion queue to be
> > > >     woken up, implying that an error (negative return code) from kernel_thread could
> > > >     lead to deadlock.  Repaired by checking return code at call time, and setting
> > > >     saved return code to zero in the event of an error.
> > > 
> > > This is inconsistent with your assertion that pid 0 "is/should be valid"
> > > above.  If you want '0' to mean "not valid" then it's not a valid return
> > > value from kernel_thread() (and arguably that's true, since pid 0 is
> > > permanently allocated to the idle thread.)
> > > 
> > No its, not, but I can see how my comments might be ambiguous. I want zero to be
> > a valid return code, since we never actually return zero, but we certainly could
> > if we wanted to.  Note that kernel_thread returns an int (not an unsigned int),
> > and as such assuming that a non-zero return code implies success ignores the
> > fact that kernel_thread can return a negative value, which indicates failure.
> > This is what I found, and what my patch fixes.
> > 
> > > I don't particularly care whether you decide to that returning pid 0 from
> > > kernel_thread is valid or not, just that your two points above are at least
> > > consistent with each other.
> > > 
> > My comments in (2) should be made more clear by changing "assume a non-zero
> > return was success" to "assume a negative return was success".  This is what my
> > patch fixes.
> 
> In the first point, you say that you want return of zero to mean success.
> In the second point, you use zero to mark an error event.  That's the
> inconsistency I'm referring to.
> 
> So, if we _did_ return zero from kernel_thread, and we had your fix as in
> (2), you'd store zero into the saved return code, which would then be
> interpreted in later code as an error.
> 
Ahh, I see what your saying now.  The later check of the stored return code
should specifically check for negative, rather than non-zero return codes,
instead of just assigning zero to the stored result to avoid the later check
going down the wrong return path, good point.  I'll fix that up early next week,
with the other items on my todo list.

Thanks & Regards
Neil

> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:  2.6 Serial core

[KJ] audit return code handling for kernel_thread [3/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 core.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -2052,11 +2052,15 @@ static CLASS_ATTR(rfcomm_dlc, S_IRUGO, r
 /* ---- Initialization ---- */
 static int __init rfcomm_init(void)
 {
+	int ret;
 	l2cap_load();
 
 	hci_register_cb(&rfcomm_cb);
 
-	kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+	ret = kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+	
+	if (ret < 0)
+		return ret;
 
 	class_create_file(bt_class, &class_attr_rfcomm_dlc);
 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [3/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 core.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -2052,11 +2052,15 @@ static CLASS_ATTR(rfcomm_dlc, S_IRUGO, r
 /* ---- Initialization ---- */
 static int __init rfcomm_init(void)
 {
+	int ret;
 	l2cap_load();
 
 	hci_register_cb(&rfcomm_cb);
 
-	kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+	ret = kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+	
+	if (ret < 0)
+		return ret;
 
 	class_create_file(bt_class, &class_attr_rfcomm_dlc);
 

[KJ] audit return code handling for kernel_thread [4/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 init/do_mounts_initrd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -57,7 +57,7 @@ static void __init handle_initrd(void)
 
 	current->flags |= PF_NOFREEZE;
 	pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
-	if (pid > 0) {
+	if (pid >= 0) {
 		while (pid != sys_wait4(-1, NULL, 0, NULL))
 			yield();
 	}
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [4/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 init/do_mounts_initrd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -57,7 +57,7 @@ static void __init handle_initrd(void)
 
 	current->flags |= PF_NOFREEZE;
 	pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
-	if (pid > 0) {
+	if (pid >= 0) {
 		while (pid != sys_wait4(-1, NULL, 0, NULL))
 			yield();
 	}

[KJ] audit return code handling for kernel_thread [5/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/s390/net/lcs.c         |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -1729,11 +1729,13 @@ lcs_start_kernel_thread(struct lcs_card 
 {
 	LCS_DBF_TEXT(5, trace, "krnthrd");
 	if (lcs_do_start_thread(card, LCS_RECOVERY_THREAD))
-		kernel_thread(lcs_recovery, (void *) card, SIGCHLD);
+		if (kernel_thread(lcs_recovery, (void *) card, SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start lcs recovery thread\n");
 #ifdef CONFIG_IP_MULTICAST
 	if (lcs_do_start_thread(card, LCS_SET_MC_THREAD))
-		kernel_thread(lcs_register_mc_addresses,
-				(void *) card, SIGCHLD);
+		if (kernel_thread(lcs_register_mc_addresses,
+				(void *) card, SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start lcs mc thread\n");
 #endif
 }
 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [5/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/s390/net/lcs.c         |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -1729,11 +1729,13 @@ lcs_start_kernel_thread(struct lcs_card 
 {
 	LCS_DBF_TEXT(5, trace, "krnthrd");
 	if (lcs_do_start_thread(card, LCS_RECOVERY_THREAD))
-		kernel_thread(lcs_recovery, (void *) card, SIGCHLD);
+		if (kernel_thread(lcs_recovery, (void *) card, SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start lcs recovery thread\n");
 #ifdef CONFIG_IP_MULTICAST
 	if (lcs_do_start_thread(card, LCS_SET_MC_THREAD))
-		kernel_thread(lcs_register_mc_addresses,
-				(void *) card, SIGCHLD);
+		if (kernel_thread(lcs_register_mc_addresses,
+				(void *) card, SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start lcs mc thread\n");
 #endif
 }
 

[KJ] audit return code handling for kernel_thread [6/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 init/main.c                   |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
--- a/init/main.c
+++ b/init/main.c
@@ -389,7 +389,10 @@ #endif
 static void noinline rest_init(void)
 	__releases(kernel_lock)
 {
-	kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
+	if (kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND) < 0) {
+		printk(KERN_CRIT "Unable to start init task!\n");
+		BUG();
+	}
 	numa_default_policy();
 	unlock_kernel();
 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [6/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 init/main.c                   |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
--- a/init/main.c
+++ b/init/main.c
@@ -389,7 +389,10 @@ #endif
 static void noinline rest_init(void)
 	__releases(kernel_lock)
 {
-	kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
+	if (kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND) < 0) {
+		printk(KERN_CRIT "Unable to start init task!\n");
+		BUG();
+	}
 	numa_default_policy();
 	unlock_kernel();
 

[KJ] audit return code handling for kernel_thread [7/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/macintosh/mediabay.c     |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/macintosh/mediabay.c
+++ b/drivers/macintosh/mediabay.c
@@ -699,7 +699,8 @@ static int __devinit media_bay_attach(st
 
 	/* Startup kernel thread */
 	if (i = 0)
-		kernel_thread(media_bay_task, NULL, CLONE_KERNEL);
+		if (kernel_thread(media_bay_task, NULL, CLONE_KERNEL) < 0)
+			printk(KERN_WARNING "Could not start media bay task\n");
 
 	return 0;
 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [7/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/macintosh/mediabay.c     |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/macintosh/mediabay.c
+++ b/drivers/macintosh/mediabay.c
@@ -699,7 +699,8 @@ static int __devinit media_bay_attach(st
 
 	/* Startup kernel thread */
 	if (i == 0)
-		kernel_thread(media_bay_task, NULL, CLONE_KERNEL);
+		if (kernel_thread(media_bay_task, NULL, CLONE_KERNEL) < 0)
+			printk(KERN_WARNING "Could not start media bay task\n");
 
 	return 0;
 

[KJ] audit return code handling for kernel_thread [8/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/ieee1394/nodemgr.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/ieee1394/nodemgr.c
+++ b/drivers/ieee1394/nodemgr.c
@@ -426,7 +426,8 @@ static ssize_t fw_set_rescan(struct bus_
 	 * something stupid and spawn this a lot of times, but that's
 	 * root's fault. */
 	if (state = 1)
-		kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL);
+		if (kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL) < 0)
+			printk(KERN_WARNING "Could not start 1394 bus rescan thread\n");
 
 	return count;
 }
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [8/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/ieee1394/nodemgr.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/ieee1394/nodemgr.c
+++ b/drivers/ieee1394/nodemgr.c
@@ -426,7 +426,8 @@ static ssize_t fw_set_rescan(struct bus_
 	 * something stupid and spawn this a lot of times, but that's
 	 * root's fault. */
 	if (state == 1)
-		kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL);
+		if (kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL) < 0)
+			printk(KERN_WARNING "Could not start 1394 bus rescan thread\n");
 
 	return count;
 }

Re: [KJ] audit return code handling for kernel_thread [8/11]

[Stefan Richter]

nhorman@tuxdriver.com wrote:
> --- a/drivers/ieee1394/nodemgr.c
> +++ b/drivers/ieee1394/nodemgr.c
> @@ -426,7 +426,8 @@ static ssize_t fw_set_rescan(struct bus_
>  	 * something stupid and spawn this a lot of times, but that's
>  	 * root's fault. */
>  	if (state = 1)
> -		kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL);
> +		if (kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL) < 0)
> +			printk(KERN_WARNING "Could not start 1394 bus rescan thread\n");
>  
>  	return count;
>  }

Thanks, but (a) we don't need the warnig and (b) we even don't need to 
spawn a thread at this point. This call to kernel_thread has been 
removed from git-ieee1394 and -mm earlier this month.
http://www.kernel.org/git/?p=linux/kernel/git/bcollins/linux1394-2.6.git;a=commitdiff_plain;h@fd89cc54a8a67c81b5aa40b22c4f40b39e47b9
http://marc.theaimsgroup.com/?l=linux-mm-commits&m\x115189722112717
-- 
Stefan Richter
-===-=-=- -== ==-http://arcgraph.de/sr/
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [8/11]

[Stefan Richter]

nhorman@tuxdriver.com wrote:
> --- a/drivers/ieee1394/nodemgr.c
> +++ b/drivers/ieee1394/nodemgr.c
> @@ -426,7 +426,8 @@ static ssize_t fw_set_rescan(struct bus_
>  	 * something stupid and spawn this a lot of times, but that's
>  	 * root's fault. */
>  	if (state == 1)
> -		kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL);
> +		if (kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL) < 0)
> +			printk(KERN_WARNING "Could not start 1394 bus rescan thread\n");
>  
>  	return count;
>  }

Thanks, but (a) we don't need the warnig and (b) we even don't need to 
spawn a thread at this point. This call to kernel_thread has been 
removed from git-ieee1394 and -mm earlier this month.
http://www.kernel.org/git/?p=linux/kernel/git/bcollins/linux1394-2.6.git;a=commitdiff_plain;h=40fd89cc54a8a67c81b5aa40b22c4f40b39e47b9
http://marc.theaimsgroup.com/?l=linux-mm-commits&m=115189722112717
-- 
Stefan Richter
-=====-=-==- -=== ===-=
http://arcgraph.de/sr/

[KJ] audit return code handling for kernel_thread [9/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/s390/net/qeth_main.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
--- a/drivers/s390/net/qeth_main.c
+++ b/drivers/s390/net/qeth_main.c
@@ -1048,11 +1048,14 @@ qeth_start_kernel_thread(struct qeth_car
 		return;
 
 	if (qeth_do_start_thread(card, QETH_SET_IP_THREAD))
-		kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD);
+		if (kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start qeth register thread\n");
 	if (qeth_do_start_thread(card, QETH_SET_PROMISC_MODE_THREAD))
-		kernel_thread(qeth_set_promisc_mode, (void *)card, SIGCHLD);
+		if (kernel_thread(qeth_set_promisc_mode, (void *)card, SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start qeth prmisc thread\n");
 	if (qeth_do_start_thread(card, QETH_RECOVER_THREAD))
-		kernel_thread(qeth_recover, (void *) card, SIGCHLD);
+		if (kernel_thread(qeth_recover, (void *) card, SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start qeth recovery thread\n");
 }
 
 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [9/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/s390/net/qeth_main.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
--- a/drivers/s390/net/qeth_main.c
+++ b/drivers/s390/net/qeth_main.c
@@ -1048,11 +1048,14 @@ qeth_start_kernel_thread(struct qeth_car
 		return;
 
 	if (qeth_do_start_thread(card, QETH_SET_IP_THREAD))
-		kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD);
+		if (kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start qeth register thread\n");
 	if (qeth_do_start_thread(card, QETH_SET_PROMISC_MODE_THREAD))
-		kernel_thread(qeth_set_promisc_mode, (void *)card, SIGCHLD);
+		if (kernel_thread(qeth_set_promisc_mode, (void *)card, SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start qeth prmisc thread\n");
 	if (qeth_do_start_thread(card, QETH_RECOVER_THREAD))
-		kernel_thread(qeth_recover, (void *) card, SIGCHLD);
+		if (kernel_thread(qeth_recover, (void *) card, SIGCHLD) < 0)
+			printk(KERN_WARNING "Could not start qeth recovery thread\n");
 }
 
 

[KJ] audit return code handling for kernel_thread [10/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/macintosh/therm_pm72.c |    4 ++++
 1 file changed, 4 insertions(+)
--- a/drivers/macintosh/therm_pm72.c
+++ b/drivers/macintosh/therm_pm72.c
@@ -1769,6 +1769,10 @@ static void start_control_loops(void)
 	init_completion(&ctrl_complete);
 
 	ctrl_task = kernel_thread(main_control_loop, NULL, SIGCHLD | CLONE_KERNEL);
+	if (ctrl_task < 0) {
+		printk(KERN_CRIT "could not start control thread\n");
+		ctrl_task = 0;
+	}
 }
 
 /*
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [10/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 drivers/macintosh/therm_pm72.c |    4 ++++
 1 file changed, 4 insertions(+)
--- a/drivers/macintosh/therm_pm72.c
+++ b/drivers/macintosh/therm_pm72.c
@@ -1769,6 +1769,10 @@ static void start_control_loops(void)
 	init_completion(&ctrl_complete);
 
 	ctrl_task = kernel_thread(main_control_loop, NULL, SIGCHLD | CLONE_KERNEL);
+	if (ctrl_task < 0) {
+		printk(KERN_CRIT "could not start control thread\n");
+		ctrl_task = 0;
+	}
 }
 
 /*

[KJ] audit return code handling for kernel_thread [11/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 arch/ia64/sn/kernel/xpc_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/ia64/sn/kernel/xpc_main.c
+++ b/arch/ia64/sn/kernel/xpc_main.c
@@ -583,7 +583,7 @@ xpc_activate_partition(struct xpc_partit
 
 	pid = kernel_thread(xpc_activating, (void *) ((u64) partid), 0);
 
-	if (unlikely(pid <= 0)) {
+	if (unlikely(pid < 0)) {
 		spin_lock_irqsave(&part->act_lock, irq_flags);
 		part->act_state = XPC_P_INACTIVE;
 		XPC_SET_REASON(part, xpcCloneKThreadFailed, __LINE__);
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] audit return code handling for kernel_thread [11/11]

[nhorman]

Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
    Problems seemed to fall into 3 main categories:
    
    1) callers of kernel_thread were inconsistent about meaning of a zero return
    code.  Some callers considered a zero return code to mean success, others took
    it to mean failure.  a zero return code, while not actually possible in the
    current implementation, should be considered a success (pid 0 is/should be
    valid). fixed all callers to treat zero return as success
    
    2) caller of kernel_thread saved return code of kernel_thread for later use
    without ever checking its value.  Callers who did this tended to assume a
    non-zero return was success, and would often wait for a completion queue to be
    woken up, implying that an error (negative return code) from kernel_thread could
    lead to deadlock.  Repaired by checking return code at call time, and setting
    saved return code to zero in the event of an error.
    
    3) callers of kernel_thread never bothered to check the return code at all.
    This can lead to seemingly unrelated errors later in execution.  Fixed by
    checking return code at call time and printing a warning message on failure.

Regards
Neil
    
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 arch/ia64/sn/kernel/xpc_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/ia64/sn/kernel/xpc_main.c
+++ b/arch/ia64/sn/kernel/xpc_main.c
@@ -583,7 +583,7 @@ xpc_activate_partition(struct xpc_partit
 
 	pid = kernel_thread(xpc_activating, (void *) ((u64) partid), 0);
 
-	if (unlikely(pid <= 0)) {
+	if (unlikely(pid < 0)) {
 		spin_lock_irqsave(&part->act_lock, irq_flags);
 		part->act_state = XPC_P_INACTIVE;
 		XPC_SET_REASON(part, xpcCloneKThreadFailed, __LINE__);

Re: [KJ] audit return code handling for kernel_thread [1/11]

[Nick Piggin]

nhorman@tuxdriver.com wrote:
> Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
>     Problems seemed to fall into 3 main categories:
>     

Thanks for doing this. Nitpick: this should be all one patch, or at most 3
patches (then each of the below 3 items would become individual changelogs).

Each patch should have a unique changelog, each should have a unique subject
(sans the sequence number).

cc'ing Andrew is also a good idea, if you want them to get merged ;)

One coding style comment:
if (...)
     multi line
         statement

Could use braces around the outermost if statement, for clarity.


>     1) callers of kernel_thread were inconsistent about meaning of a zero return
>     code.  Some callers considered a zero return code to mean success, others took
>     it to mean failure.  a zero return code, while not actually possible in the
>     current implementation, should be considered a success (pid 0 is/should be
>     valid). fixed all callers to treat zero return as success
>     
>     2) caller of kernel_thread saved return code of kernel_thread for later use
>     without ever checking its value.  Callers who did this tended to assume a
>     non-zero return was success, and would often wait for a completion queue to be
>     woken up, implying that an error (negative return code) from kernel_thread could
>     lead to deadlock.  Repaired by checking return code at call time, and setting
>     saved return code to zero in the event of an error.
>     
>     3) callers of kernel_thread never bothered to check the return code at all.
>     This can lead to seemingly unrelated errors later in execution.  Fixed by
>     checking return code at call time and printing a warning message on failure.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [1/11]

[Nick Piggin]

nhorman@tuxdriver.com wrote:
> Audit/Cleanup of kernel_thread calls, specifically checking of return codes.
>     Problems seemed to fall into 3 main categories:
>     

Thanks for doing this. Nitpick: this should be all one patch, or at most 3
patches (then each of the below 3 items would become individual changelogs).

Each patch should have a unique changelog, each should have a unique subject
(sans the sequence number).

cc'ing Andrew is also a good idea, if you want them to get merged ;)

One coding style comment:
if (...)
     multi line
         statement

Could use braces around the outermost if statement, for clarity.


>     1) callers of kernel_thread were inconsistent about meaning of a zero return
>     code.  Some callers considered a zero return code to mean success, others took
>     it to mean failure.  a zero return code, while not actually possible in the
>     current implementation, should be considered a success (pid 0 is/should be
>     valid). fixed all callers to treat zero return as success
>     
>     2) caller of kernel_thread saved return code of kernel_thread for later use
>     without ever checking its value.  Callers who did this tended to assume a
>     non-zero return was success, and would often wait for a completion queue to be
>     woken up, implying that an error (negative return code) from kernel_thread could
>     lead to deadlock.  Repaired by checking return code at call time, and setting
>     saved return code to zero in the event of an error.
>     
>     3) callers of kernel_thread never bothered to check the return code at all.
>     This can lead to seemingly unrelated errors later in execution.  Fixed by
>     checking return code at call time and printing a warning message on failure.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [KJ] audit return code handling for kernel_thread [1/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 09:49:01AM +1000, Nick Piggin wrote:
> nhorman@tuxdriver.com wrote:
> >Audit/Cleanup of kernel_thread calls, specifically checking of return 
> >codes.
> >    Problems seemed to fall into 3 main categories:
> >    
> 
> Thanks for doing this. Nitpick: this should be all one patch, or at most 3
> patches (then each of the below 3 items would become individual changelogs).
> 
You're welcome. I specifically split it into multiple little patches, as each file has a
different maintainer, but if the consensus is for one patch (or three), so be it, I'll do
that in the future.

> Each patch should have a unique changelog, each should have a unique subject
> (sans the sequence number).
> 
> cc'ing Andrew is also a good idea, if you want them to get merged ;)
> 
I can do that :)

> One coding style comment:
> if (...)
>     multi line
>         statement
> 
> Could use braces around the outermost if statement, for clarity.
> 
If you ack this, I'll post a follow on patch to clean that up next week.  I've already
received a suggestion to use the same failure to start thread warning message to
save string table space, so I've got some extra clean up to do anyway.

Regards
Neil

> 
> >    1) callers of kernel_thread were inconsistent about meaning of a zero 
> >    return
> >    code.  Some callers considered a zero return code to mean success, 
> >    others took
> >    it to mean failure.  a zero return code, while not actually possible 
> >    in the
> >    current implementation, should be considered a success (pid 0 
> >    is/should be
> >    valid). fixed all callers to treat zero return as success
> >    
> >    2) caller of kernel_thread saved return code of kernel_thread for 
> >    later use
> >    without ever checking its value.  Callers who did this tended to 
> >    assume a
> >    non-zero return was success, and would often wait for a completion 
> >    queue to be
> >    woken up, implying that an error (negative return code) from 
> >    kernel_thread could
> >    lead to deadlock.  Repaired by checking return code at call time, and 
> >    setting
> >    saved return code to zero in the event of an error.
> >    
> >    3) callers of kernel_thread never bothered to check the return code at 
> >    all.
> >    This can lead to seemingly unrelated errors later in execution.  Fixed 
> >    by
> >    checking return code at call time and printing a warning message on 
> >    failure.
> 
> -- 
> SUSE Labs, Novell Inc.
> Send instant messages to your online friends http://au.messenger.yahoo.com 

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] audit return code handling for kernel_thread [1/11]

[Neil Horman]

On Sat, Jul 29, 2006 at 09:49:01AM +1000, Nick Piggin wrote:
> nhorman@tuxdriver.com wrote:
> >Audit/Cleanup of kernel_thread calls, specifically checking of return 
> >codes.
> >    Problems seemed to fall into 3 main categories:
> >    
> 
> Thanks for doing this. Nitpick: this should be all one patch, or at most 3
> patches (then each of the below 3 items would become individual changelogs).
> 
You're welcome. I specifically split it into multiple little patches, as each file has a
different maintainer, but if the consensus is for one patch (or three), so be it, I'll do
that in the future.

> Each patch should have a unique changelog, each should have a unique subject
> (sans the sequence number).
> 
> cc'ing Andrew is also a good idea, if you want them to get merged ;)
> 
I can do that :)

> One coding style comment:
> if (...)
>     multi line
>         statement
> 
> Could use braces around the outermost if statement, for clarity.
> 
If you ack this, I'll post a follow on patch to clean that up next week.  I've already
received a suggestion to use the same failure to start thread warning message to
save string table space, so I've got some extra clean up to do anyway.

Regards
Neil

> 
> >    1) callers of kernel_thread were inconsistent about meaning of a zero 
> >    return
> >    code.  Some callers considered a zero return code to mean success, 
> >    others took
> >    it to mean failure.  a zero return code, while not actually possible 
> >    in the
> >    current implementation, should be considered a success (pid 0 
> >    is/should be
> >    valid). fixed all callers to treat zero return as success
> >    
> >    2) caller of kernel_thread saved return code of kernel_thread for 
> >    later use
> >    without ever checking its value.  Callers who did this tended to 
> >    assume a
> >    non-zero return was success, and would often wait for a completion 
> >    queue to be
> >    woken up, implying that an error (negative return code) from 
> >    kernel_thread could
> >    lead to deadlock.  Repaired by checking return code at call time, and 
> >    setting
> >    saved return code to zero in the event of an error.
> >    
> >    3) callers of kernel_thread never bothered to check the return code at 
> >    all.
> >    This can lead to seemingly unrelated errors later in execution.  Fixed 
> >    by
> >    checking return code at call time and printing a warning message on 
> >    failure.
> 
> -- 
> SUSE Labs, Novell Inc.
> Send instant messages to your online friends http://au.messenger.yahoo.com 

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/