[PATCH] Infinite loop/always true check possible with unsigned counter.

[PATCH] Infinite loop/always true check possible with unsigned counter.

[Manish Ahuja]

[-- Attachment #1: Type: text/plain, Size: 239 bytes --]

Fix to correct a possible infinite loop or an always true check when the 
unsigned long counter "i" is used in
lmb_add_region() in the following for loop:

for (i = rgn->cnt-1; i >= 0; i--)

Signed-off-by: Manish Ahuja <ahuja@us.ibm.com>


[-- Attachment #2: unsigned --]
[-- Type: text/plain, Size: 698 bytes --]

---
 arch/powerpc/mm/lmb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: 2.6.22-rc4/arch/powerpc/mm/lmb.c
===================================================================
--- 2.6.22-rc4.orig/arch/powerpc/mm/lmb.c	2007-06-11 21:10:46.000000000 -0500
+++ 2.6.22-rc4/arch/powerpc/mm/lmb.c	2007-07-06 21:47:40.000000000 -0500
@@ -138,8 +138,8 @@ void __init lmb_analyze(void)
 static long __init lmb_add_region(struct lmb_region *rgn, unsigned long base,
 				  unsigned long size)
 {
-	unsigned long i, coalesced = 0;
-	long adjacent;
+	unsigned long coalesced = 0;
+	long adjacent, i;
 
 	/* First try and coalesce this LMB with another. */
 	for (i=0; i < rgn->cnt; i++) {

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Manish Ahuja]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

Repost to fix my email id.

Fix to correct a possible infinite loop or an always true check when the 
unsigned long counter "i" is used in
lmb_add_region() in the following for loop:

for (i = rgn->cnt-1; i >= 0; i--)

Signed-off-by: Manish Ahuja <ahuja@austin.ibm.com>



[-- Attachment #2: unsigned --]
[-- Type: text/plain, Size: 698 bytes --]

---
 arch/powerpc/mm/lmb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: 2.6.22-rc4/arch/powerpc/mm/lmb.c
===================================================================
--- 2.6.22-rc4.orig/arch/powerpc/mm/lmb.c	2007-06-11 21:10:46.000000000 -0500
+++ 2.6.22-rc4/arch/powerpc/mm/lmb.c	2007-07-06 21:47:40.000000000 -0500
@@ -138,8 +138,8 @@ void __init lmb_analyze(void)
 static long __init lmb_add_region(struct lmb_region *rgn, unsigned long base,
 				  unsigned long size)
 {
-	unsigned long i, coalesced = 0;
-	long adjacent;
+	unsigned long coalesced = 0;
+	long adjacent, i;
 
 	/* First try and coalesce this LMB with another. */
 	for (i=0; i < rgn->cnt; i++) {

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Paul Mackerras]

Manish Ahuja writes:

> Repost to fix my email id.
> 
> Fix to correct a possible infinite loop or an always true check when the 
> unsigned long counter "i" is used in
> lmb_add_region() in the following for loop:
> 
> for (i = rgn->cnt-1; i >= 0; i--)

Unfortunately this won't fix the bug.  Since rgn->cnt is unsigned
long, the comparison will be done as an unsigned comparison even if i
is signed.

Instead, change the loop to go from rgn->cnt down to 1, and in the
loop body, change all instances of "i+1" to "i", and "i" to "i-1".

Paul.

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Felix Domke]

Paul Mackerras wrote:
>> Fix to correct a possible infinite loop or an always true check when the 
>> unsigned long counter "i" is used in
>> lmb_add_region() in the following for loop:
>> for (i = rgn->cnt-1; i >= 0; i--)
> Unfortunately this won't fix the bug.  Since rgn->cnt is unsigned
> long, the comparison will be done as an unsigned comparison even if i
> is signed.
> Instead, change the loop to go from rgn->cnt down to 1, and in the
> loop body, change all instances of "i+1" to "i", and "i" to "i-1".
What's about

i = rgn->cnt;
while (i--)
...
?

(or  for (i=rgn->cnt; i--; ), if you want a for-loop)


Felix

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Andreas Schwab]

Paul Mackerras <paulus@samba.org> writes:

> Manish Ahuja writes:
>
>> Repost to fix my email id.
>> 
>> Fix to correct a possible infinite loop or an always true check when the 
>> unsigned long counter "i" is used in
>> lmb_add_region() in the following for loop:
>> 
>> for (i = rgn->cnt-1; i >= 0; i--)
>
> Unfortunately this won't fix the bug.  Since rgn->cnt is unsigned
> long, the comparison will be done as an unsigned comparison even if i
> is signed.

??? There is no rgn->cnt involved in the comparison.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Paul Mackerras]

Andreas Schwab writes:
> Paul Mackerras <paulus@samba.org> writes:
> 
> > Manish Ahuja writes:
> >
> >> Repost to fix my email id.
> >> 
> >> Fix to correct a possible infinite loop or an always true check when the 
> >> unsigned long counter "i" is used in
> >> lmb_add_region() in the following for loop:
> >> 
> >> for (i = rgn->cnt-1; i >= 0; i--)
> >
> > Unfortunately this won't fix the bug.  Since rgn->cnt is unsigned
> > long, the comparison will be done as an unsigned comparison even if i
> > is signed.
> 
> ??? There is no rgn->cnt involved in the comparison.

Look further down in lmb_add_region; there is a second for loop that
does

	for (i = rgn->cnt-1; i >= 0; i--)

I agree that Manish's patch description was misleading, but there is
a real problem in there.

Paul.

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Andreas Schwab]

Paul Mackerras <paulus@samba.org> writes:

> Andreas Schwab writes:
>> Paul Mackerras <paulus@samba.org> writes:
>> 
>> > Manish Ahuja writes:
>> >
>> >> Repost to fix my email id.
>> >> 
>> >> Fix to correct a possible infinite loop or an always true check when the 
>> >> unsigned long counter "i" is used in
>> >> lmb_add_region() in the following for loop:
>> >> 
>> >> for (i = rgn->cnt-1; i >= 0; i--)
>> >
>> > Unfortunately this won't fix the bug.  Since rgn->cnt is unsigned
>> > long, the comparison will be done as an unsigned comparison even if i
>> > is signed.
>> 
>> ??? There is no rgn->cnt involved in the comparison.
>
> Look further down in lmb_add_region; there is a second for loop that
> does
>
> 	for (i = rgn->cnt-1; i >= 0; i--)

Which is exactly the one quoted above.  I still don't see your point.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Paul Mackerras]

Andreas Schwab writes:

> >> ??? There is no rgn->cnt involved in the comparison.
> >
> > Look further down in lmb_add_region; there is a second for loop that
> > does
> >
> > 	for (i = rgn->cnt-1; i >= 0; i--)
> 
> Which is exactly the one quoted above.  I still don't see your point.

You're right - my mistake.

Paul.

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Manish Ahuja]

Paul Mackerras wrote:
> Andreas Schwab writes:
>
>   
>>>> ??? There is no rgn->cnt involved in the comparison.
>>>>         
>>> Look further down in lmb_add_region; there is a second for loop that
>>> does
>>>
>>> 	for (i = rgn->cnt-1; i >= 0; i--)
>>>       
>> Which is exactly the one quoted above.  I still don't see your point.
>>     
>
> You're right - my mistake.
>
> Paul.
>   
I presume the patch is good then. Do I need to change anything ?

Thanks,
Manish

Re: [PATCH] Infinite loop/always true check possible with unsigned counter.

[Paul Mackerras]

Manish Ahuja writes:

> I presume the patch is good then. Do I need to change anything ?

I guess not.  It will cause a warning on the first for loop if anyone
tries to compile with -Wextra or -Wsign-compare, but it would be only
one of lots of warnings in that case (and in fact comparing signed
with unsigned is a perfectly legitimate, well-defined thing to do; you
just have to be aware that the comparison is done as unsigned).

Paul.