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

From: Ingo Molnar
Date: Thu Feb 07 2008 - 05:17:20 EST



* Yinghai Lu <yhlu.kernel@xxxxxxxxx> wrote:

> On Feb 7, 2008 1:04 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
> >
> > * Yinghai Lu <yhlu.kernel@xxxxxxxxx> 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
--
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/