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

[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

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

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

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