lkml.org 
[lkml]   [2008]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [git head] X86_PAT & mprotect
On Wed, May 07, 2008 at 09:02:17AM +0200, Ingo Molnar wrote:
>
> * Venki Pallipadi <venkatesh.pallipadi@intel.com> wrote:
>
> > There is a hole in mprotect, which lets the user to change the page
> > cache type bits by-passing the kernel reserve_memtype and free_memtype
> > wrappers. Fix the hole by not letting mprotect change the PAT bits.
> >
> > Some versions of X used the mprotect hole to change caching type from
> > UC to WB, so that it can then use mtrr to program WC for that region
> > [1]. Change the mmap of pci space through /sys or /proc interfaces
> > from UC to UC_MINUS. With this change, X will not need to use mprotect
> > hole to get WC type.
> >
> > [1] lkml.org/lkml/2008/4/16/369
> >
> > Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> >
> > ---
> > arch/x86/pci/i386.c | 4 +---
> > include/asm-x86/pgtable.h | 5 ++++-
> > include/linux/mm.h | 1 +
> > mm/mmap.c | 13 +++++++++++++
> > mm/mprotect.c | 4 +++-
> > 5 files changed, 22 insertions(+), 5 deletions(-)
>
> hm, that's one dangerous looking patch. (Cc:-ed more MM folks. I've
> attached the patch below for reference.)
>
> the purpose of the fix itself seems to make some sense - we dont want
> mprotect() change the PAT bits in the pte from what they got populated
> with at fault or mmap time.
>
> the pte_modify() change looks correct at first sight.
>
> The _PAGE_PROT_PRESERVE_BITS solution looks a bit ugly (although we do
> have a couple of other similar #ifndefs in the MM already).
>
> at minimum we should add vm_get_page_prot_preserve() as an inline
> function to mm.h if _PAGE_PROT_PRESERVE_BITS is not defined, and make it
> call vm_get_page_prot(). Also, vm_get_page_prot() in mm/mmap.c should
> probably be marked inline so that we'll have only a single function call
> [vm_get_page_prot() is trivial].

I did check that with _PAGE_PROT_PRESERVE_BITS defined to zero,
compiler optimizes vm_get_page_prot_preserve to generate same code as
vm_get_page_prot with no function call (on x86 64). So, things should be OK
from overhead perspective. But, from the code cleanliness aspect,
PRESERVE_BITS looks unclean and needs some cleanup. The reason I posted the
patch as is, was to get the confirmation on the original thread, whether this
indeed fixes those PAT related error messages.

> but i'm wondering why similar issues never came up on other
> architectures - i thought it would be rather common to have immutable
> pte details. So maybe i'm missing something here ...

Probably other architectures does not depend on preserving things in
vma->vm_page_prot once ptes are set correctly. With PAT, we use
vm_page_prot to keep track of PAT attributes for vmas from parent to
child across fork.

pte_modify() part will be required in all archs that wants to preserve
some bits in pte in a mprotect call.

Thanks,
Venki



\
 
 \ /
  Last update: 2008-05-08 00:39    [W:0.092 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site