LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Andi Kleen <ak@suse.de>
Cc: mingo@elte.hu, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] [3/5] CPA: Make advised protection check truly advisory
Date: Sat, 9 Feb 2008 18:38:50 +0100 (CET)	[thread overview]
Message-ID: <alpine.LFD.1.00.0802091809480.3145@apollo.tec.linutronix.de> (raw)
In-Reply-To: <200802091756.06553.ak@suse.de>

On Sat, 9 Feb 2008, Andi Kleen wrote:
> On Saturday 09 February 2008 16:38:35 Thomas Gleixner wrote:
> > On Fri, 8 Feb 2008, Andi Kleen wrote:
> > 
> > > 
> > > Only force RO in the advisory protection checks when all pages in the 
> > > range are RO. Previously it would trigger when any page in the range
> > > was ro.
> > > 
> > > I believe this will make try_preserve_large_page much safer to use.
> > 
> > It might be quite useful to know it for sure.
> 
> I wrote that originally when you still had the bogus "AMD fix" 
> in the tree.  I think it was because I hoped it would fix that one,
> but I wasn't sure because I couldn't reproduce it. But Hugh's
> patch fixed that anyways.

The AMD limiting was due to a testing failure on a 64bit X2 and had
nothing to do with the 32bit wreckage, which was fixed by Hugh. 

The fix for the X2 was a missing check for large pmds/puds in the
spurious fault code.

> > Thinking about the whole set of changes, you are right, that the
> > current check for the first page only is not correct, but I don't see
> > how your changes make it more correct.
> 
> With my patch at least NX should be always correct and that is 
> the more important one because it is default and has to be cleared
> and things go horribly wrong when it is incorrect.
> 
> RO on the other hand defaults to off and is only sometimes forced.
>  
> > The correct way to fix this is to check, whether all the small pages,
> > which fit in the range of the large page, which we want to preserve,
> > have the same resulting pgprot flags.
> 
> Ok if you don't like the try_preserve_large_page change
> feel free to drop it or implement it differently.
> 
> My main goal here was to clean up the 32bit direct mapping
> anyways (last patch) and that does just require splitting out the 
> NX checks from the RO checks and having a range (first patches)
> 
> I think at least the range checks are definitely needed
> for correctness though.
> 
> If I cared particularly I would probably implement two modi: 
> one with DEBUG_RODATA and another without. With DEBUG_RODATA 
> advisory protections should be forced (and large pages split),
> without not.

I can understand, what you want to achieve, but I really do not like
the result for following reasons:

1) If there is a bug in some code, then we fix the bug and do not
intermingle it with something else.

2) I care about RO as much as I care about the NX correctness. That's
the same logic and the same problem. If we have overlapping regions,
then we need to split large pages. Otherwise both protections are
useless to a certain degree.

3) For correctness reasons I even ponder to make the NX/RO mandatory.

Thanks,
	tglx

  reply	other threads:[~2008-02-09 17:39 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-08 16:36 [PATCH] [0/5] pageattr protection patchkit v2 for the latest kernel Andi Kleen
2008-02-08 16:36 ` [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
2008-02-09 14:56   ` Thomas Gleixner
2008-02-09 15:13     ` Andi Kleen
2008-02-09 15:50       ` Thomas Gleixner
2008-02-09 16:39         ` Andi Kleen
2008-02-09 17:09           ` Thomas Gleixner
2008-02-10  9:39             ` Andi Kleen
2008-02-08 16:36 ` [PATCH] [2/5] Support range checking for required/advisory protections Andi Kleen
2008-02-08 16:36 ` [PATCH] [3/5] CPA: Make advised protection check truly advisory Andi Kleen
2008-02-09 15:38   ` Thomas Gleixner
2008-02-09 16:56     ` Andi Kleen
2008-02-09 17:38       ` Thomas Gleixner [this message]
2008-02-10  9:19         ` Andi Kleen
2008-02-10 16:50           ` Thomas Gleixner
2008-02-08 16:36 ` [PATCH] [4/5] Don't use inline for the protection checks Andi Kleen
2008-02-08 16:36 ` [PATCH] [5/5] Switch i386 early boot page table initialization over to use required_static_prot() Andi Kleen
  -- strict thread matches above, loose matches on Subject: below --
2008-02-08 13:27 [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
2008-02-08 13:27 ` [PATCH] [3/5] CPA: Make advised protection check truly advisory Andi Kleen

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=alpine.LFD.1.00.0802091809480.3145@apollo.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=ak@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --subject='Re: [PATCH] [3/5] CPA: Make advised protection check truly advisory' \
    /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).