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

["Petr Vandrovec" <VANDROVE@vc.cvut.cz>]

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@mail.ee>
> > 
> > --- 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'.  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, 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.
                                                     Best regards,
                                                            Petr Vandrovec