LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ingo Molnar <mingo@elte.hu>
To: Yinghai Lu <yhlu.kernel@gmail.com>
Cc: Balaji Rao <balajirrao@gmail.com>,
	linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	jesse.barnes@intel.com, ak@suse.de,
	Harvey Harrison <harvey.harrison@gmail.com>,
	"H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH][Regression] x86, 32-bit: trim memory not covered by wb mtrrs - FIX
Date: Thu, 7 Feb 2008 11:16:42 +0100	[thread overview]
Message-ID: <20080207101642.GC7716@elte.hu> (raw)
In-Reply-To: <86802c440802070111o5a4bc700g75a0693f8307d766@mail.gmail.com>


* 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

  reply	other threads:[~2008-02-07 10:17 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-07  7:27 Balaji Rao
2008-02-07  8:02 ` Ingo Molnar
2008-02-07  8:21   ` Balaji Rao
2008-02-07  8:50     ` Yinghai Lu
2008-02-07  9:04       ` Ingo Molnar
2008-02-07  9:11         ` Yinghai Lu
2008-02-07 10:16           ` Ingo Molnar [this message]
2008-02-07 11:35       ` Balaji Rao
2008-02-07  8:56   ` Yinghai Lu
2008-02-07  9:00     ` Ingo Molnar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080207101642.GC7716@elte.hu \
    --to=mingo@elte.hu \
    --cc=ak@suse.de \
    --cc=balajirrao@gmail.com \
    --cc=harvey.harrison@gmail.com \
    --cc=hpa@zytor.com \
    --cc=jesse.barnes@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=yhlu.kernel@gmail.com \
    --subject='Re: [PATCH][Regression] x86, 32-bit: trim memory not covered by wb mtrrs - FIX' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).