* Re: [PATCH 2.6] clean-up: fixes "comparison between signed
@ 2004-12-06 21:37 Petr Vandrovec
2004-12-06 23:09 ` Jesper Juhl
0 siblings, 1 reply; 6+ messages in thread
From: Petr Vandrovec @ 2004-12-06 21:37 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-kernel, mroos
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2.6] clean-up: fixes "comparison between signed 2004-12-06 21:37 [PATCH 2.6] clean-up: fixes "comparison between signed Petr Vandrovec @ 2004-12-06 23:09 ` Jesper Juhl 2004-12-07 1:02 ` Petr Vandrovec 0 siblings, 1 reply; 6+ messages in thread From: Jesper Juhl @ 2004-12-06 23:09 UTC (permalink / raw) To: Petr Vandrovec; +Cc: linux-kernel, mroos, Riina Kikas 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@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'. 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@dif.dk> 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)) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6] clean-up: fixes "comparison between signed 2004-12-06 23:09 ` Jesper Juhl @ 2004-12-07 1:02 ` Petr Vandrovec 2004-12-07 23:20 ` Jesper Juhl 0 siblings, 1 reply; 6+ messages in thread From: Petr Vandrovec @ 2004-12-07 1:02 UTC (permalink / raw) To: Jesper Juhl; +Cc: linux-kernel, mroos, Riina Kikas On Tue, Dec 07, 2004 at 12:09:05AM +0100, Jesper Juhl wrote: > On Mon, 6 Dec 2004, Petr Vandrovec wrote: > > 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? Yes. Though I believe that we already take vma == NULL path when address is that big. Petr Vandrovec > Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk> > > 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)) > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6] clean-up: fixes "comparison between signed 2004-12-07 1:02 ` Petr Vandrovec @ 2004-12-07 23:20 ` Jesper Juhl 2004-12-07 23:36 ` Petr Vandrovec 0 siblings, 1 reply; 6+ messages in thread From: Jesper Juhl @ 2004-12-07 23:20 UTC (permalink / raw) To: Petr Vandrovec; +Cc: linux-kernel, mroos, Riina Kikas On Tue, 7 Dec 2004, Petr Vandrovec wrote: > On Tue, Dec 07, 2004 at 12:09:05AM +0100, Jesper Juhl wrote: > > On Mon, 6 Dec 2004, Petr Vandrovec wrote: > > > 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? > > Yes. Though I believe that we already take vma == NULL path when address is that big. Hmm, where? - maybe I'm blind or just stupid, but I don't seem to be able to find where we do that. And would it hurt to have that additional check there as well in case address was modified after the previous check and before being passed to do_page_fault ? (note: I'm writing this last bit without having mined the source for info yet). -- Jesper ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6] clean-up: fixes "comparison between signed 2004-12-07 23:20 ` Jesper Juhl @ 2004-12-07 23:36 ` Petr Vandrovec 2004-12-08 22:24 ` Jesper Juhl 0 siblings, 1 reply; 6+ messages in thread From: Petr Vandrovec @ 2004-12-07 23:36 UTC (permalink / raw) To: Jesper Juhl; +Cc: linux-kernel, mroos, Riina Kikas On Wed, Dec 08, 2004 at 12:20:01AM +0100, Jesper Juhl wrote: > On Tue, 7 Dec 2004, Petr Vandrovec wrote: > > > On Tue, Dec 07, 2004 at 12:09:05AM +0100, Jesper Juhl wrote: > > > On Mon, 6 Dec 2004, Petr Vandrovec wrote: > > > > 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? > > > > Yes. Though I believe that we already take vma == NULL path when address is that big. > > Hmm, where? - maybe I'm blind or just stupid, but I don't seem to be able > to find where we do that. > And would it hurt to have that additional check there as well in case > address was modified after the previous check and before being passed to > do_page_fault ? (note: I'm writing this last bit without having mined the > source for info yet). If find_vma() returns NULL, it is bad_area, and no further tests occur. Otherwise if vma->vm_start <= address, it is good area. Only when these two conditions are satisifed (find_vma found vma, and this vma begins above vma's vm_start, regs->esp is checked. And as vma->vm_start can be at most 0xFFFFF000 (it is page aligned, and you cannot have vma at 4GB - actually you cannot have vma above 3GB on normal kernel, or 4GB-<whatever>MB on 4G/4G kernel), there is no way how 'address' could be in top 4KB, and so adding 32 to it cannot overflow 32bit variable. At least I believe this... Petr Vandrovec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6] clean-up: fixes "comparison between signed 2004-12-07 23:36 ` Petr Vandrovec @ 2004-12-08 22:24 ` Jesper Juhl 0 siblings, 0 replies; 6+ messages in thread From: Jesper Juhl @ 2004-12-08 22:24 UTC (permalink / raw) To: Petr Vandrovec; +Cc: linux-kernel, mroos, Riina Kikas On Wed, 8 Dec 2004, Petr Vandrovec wrote: > On Wed, Dec 08, 2004 at 12:20:01AM +0100, Jesper Juhl wrote: > > On Tue, 7 Dec 2004, Petr Vandrovec wrote: > > > > > On Tue, Dec 07, 2004 at 12:09:05AM +0100, Jesper Juhl wrote: > > > > On Mon, 6 Dec 2004, Petr Vandrovec wrote: > > > > > 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? > > > > > > Yes. Though I believe that we already take vma == NULL path when address is that big. > > > > Hmm, where? - maybe I'm blind or just stupid, but I don't seem to be able > > to find where we do that. > > And would it hurt to have that additional check there as well in case > > address was modified after the previous check and before being passed to > > do_page_fault ? (note: I'm writing this last bit without having mined the > > source for info yet). > > If find_vma() returns NULL, it is bad_area, and no further tests occur. Otherwise > if vma->vm_start <= address, it is good area. > > Only when these two conditions are satisifed (find_vma found vma, and this vma begins > above vma's vm_start, regs->esp is checked. And as vma->vm_start can be at most > 0xFFFFF000 (it is page aligned, and you cannot have vma at 4GB - actually you cannot > have vma above 3GB on normal kernel, or 4GB-<whatever>MB on 4G/4G kernel), there is > no way how 'address' could be in top 4KB, and so adding 32 to it cannot overflow > 32bit variable. > > At least I believe this... Having read your explanation (thank you) and re-read the code I think you are right. If that's indeed the case, then it would seem that the code is just fine and the best we can do is just add an explicit cast to unsigned long to shut up the compiler. Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk> 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-08 23:14:59.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) goto bad_area; } if (expand_stack(vma, address)) If you agree I'll forward the patch to Andrew. -- Jesper Juhl ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-12-08 22:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-12-06 21:37 [PATCH 2.6] clean-up: fixes "comparison between signed Petr Vandrovec 2004-12-06 23:09 ` Jesper Juhl 2004-12-07 1:02 ` Petr Vandrovec 2004-12-07 23:20 ` Jesper Juhl 2004-12-07 23:36 ` Petr Vandrovec 2004-12-08 22:24 ` Jesper Juhl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox