Re: [PATCH 2.6] clean-up: fixes "comparison between signed

[Jesper Juhl]

On Mon, 6 Dec 2004, Petr Vandrovec wrote:

> On  6 Dec 04 at 23:11, Jesper Juhl wrote:
> > On Mon, 6 Dec 2004, Riina Kikas wrote:
> > 
> > > This patch fixes warning "comparison between signed and unsigned"
> > > occuring on line 308
> > > 
> > > Signed-off-by: Riina Kikas <Riina.Kikas@xxxxxxx>
> > > 
> > > --- a/arch/i386/mm/fault.c    2004-12-02 21:30:30.000000000 +0000
> > > +++ b/arch/i386/mm/fault.c    2004-12-02 21:30:59.000000000 +0000
> > > @@ -302,7 +302,13 @@
> > >        * pusha) doing post-decrement on the stack and that
> > >        * doesn't show up until later..
> > >        */
> > > -     if (address + 32 < regs->esp)
> > > +     unsigned long regs_esp;
> > > +     if (regs->esp < 0) {
> > > +         regs_esp = 0;
> > > +     } else {
> > > +         regs_esp = regs->esp;
> > > +     }
> > > +     if (address + 32 < regs_esp)
> > >           goto bad_area;
> > >   }
> > >   if (expand_stack(vma, address))
> > 
> > This seems a bit silly. If the stack pointer (esp) is ever negative that's 
> > clearly a bug somewhere. So instead of testing it for <0 and then setting 
> > your regs_esp variable to 0 it would make more sense to me to just 
> > BUG_ON(regs->esp < 0) or something, if you want to do anything at all. And 
> > if you want to silence the warning a exlicit cast to unsigned long should 
> > do I'd say, but I have a feeling the best thing is to just leave that 
> > warning alone, the code seems to be fine.
> 
> regs->esp is < 0 almost always - user stack starts at 0xBFFFFFFF, which
> is negative number when you treat it as 'long'.  

True, if you treat that nr as signed long it's clearly negative. I guess 
what twisted my head there was thinking about a register as a signed C 
type.
Why are the registers not defined as unsigned types in the struct in the 
first place?

>Change is wrong, now
> when esp is in top 2GB 'address' is never evaluated as invalid, and it
> was definitely not intention.
> 
> Correct is (if any fix is needed at all) typecast regs->esp to unsigned
> long, 

That would have been my suggestion as well.

>eventually with check that address is less than (unsigned long)-32,
> as area at VA 0 is not going to grow "down" to 0xFFFFFxxx, even if you
> nicely ask.

you mean something like this - right?

Signed-off-by: Jesper Juhl <juhl-lkml@xxxxxx>

diff -up linux-2.6.10-rc3-bk2-orig/arch/i386/mm/fault.c linux-2.6.10-rc3-bk2/arch/i386/mm/fault.c
--- linux-2.6.10-rc3-bk2-orig/arch/i386/mm/fault.c	2004-12-06 22:24:16.000000000 +0100
+++ linux-2.6.10-rc3-bk2/arch/i386/mm/fault.c	2004-12-07 00:04:33.000000000 +0100
@@ -305,7 +305,7 @@ fastcall void do_page_fault(struct pt_re
 		 * pusha) doing post-decrement on the stack and that
 		 * doesn't show up until later..
 		 */
-		if (address + 32 < regs->esp)
+		if (address + 32 < (unsigned long)regs->esp || address >= (~0UL - 32))
 			goto bad_area;
 	}
 	if (expand_stack(vma, address))


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/