Re: [PATCH][Regression] x86, 32-bit: trim memory not covered by wb mtrrs - FIX

[Ingo Molnar <mingo@elte.hu>]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> On Feb 7, 2008 1:04 AM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> >
> > > minor difference
> > > +               trim_start = highest_pfn << PAGE_SHIFT;
> > > +               trim_size = end_pfn << PAGE_SHIFT;
> > >
> > > could cause some problem with 32 bit kernel when mem > 4g. becase
> > > highest_pfn and end_pfn is unsigned long aka 32 bit ...could overflow.
> > >
> > > so need to assign thtem to tr, 32-bitim_start/trim_end at first
> > > or
> > > +               trim_start = (u64)highest_pfn << PAGE_SHIFT;
> > > +               trim_size = (u64)end_pfn << PAGE_SHIFT;
> >
> > indeed ...
> >
> > i think the 64-bit behavior of gcc is inherently dangerous, we had
> > numerous subtle bugs in that area.
> >
> > i think perhaps Sparse should be extended to warn about this. I think
> > any case where on _32-bit_ we have an 'unsigned long' that is shifted to
> > the left by any significant amount is clearly in danger of overflowing.
> > _Especially_ when the lvalue is 64-bit!
> >
> > or in other words, on any such construct:
> >
> >    64-bit lvalue = ... 32-bit value
> >
> > we should enforce _explicit_ (u64) conversions.
> 
> so you mean gcc will do some optimization to make
> 
> +               trim_start = highest_pfn;
> +               trim_start <<= PAGE_SHIFT;
> 
> to be
> 
> +               trim_start = highest_pfn << PAGE_SHIFT;
> 
> looks scary...

no, that would not be valid.

I mean the simple example you offered:

  +               trim_start = highest_pfn << PAGE_SHIFT;

it _looks_ good but is inherently unsafe if 'highest_pfn' is 32-bit and 
PAGE_SHIFT is 32-bit (which they are).

Or another, user-space example, on a 32-bit box:

 int main (void)
 {
         unsigned long long a;
         unsigned long b = 0x80000000, c = 2;

         a = b << c;

         printf("%Ld\n", a);

         return 0;
 }

gcc will print "0" and emits no warning - so we silently lost 
information and truncated bits. I'm sure this is somewhere specified in 
some language standard, but it's causing bugs left and right when we use 
u64.

So if there's no helpful gcc warning that already exists, i think we 
should extend the Sparse static checker to flag all such instances of:

    u64 = u32 << u32;

arithmetics, because in 100% of the cases i've seen so far they cause 
nasty bugs. We've had such bugs in the scheduler, and we've had them in 
arch/x86 as well. It's a royal PITA.

	Ingo