Re: [PATCH] [3/5] CPA: Make advised protection check truly advisory

[Andi Kleen]

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.

> 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.

-Andi
--
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/