Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1317097 > unrolled thread

[PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap() Matthew Wilcox <matthew.r.wilcox@intel.com> - 2016-01-25 18:30 +0100
    Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap() Andy Lutomirski <luto@amacapital.net> - 2016-01-25 18:40 +0100
      Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap() Andy Lutomirski <luto@amacapital.net> - 2016-01-25 18:50 +0100
      Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and  track_pfn_remap() Matthew Wilcox <willy@linux.intel.com> - 2016-01-27 05:50 +0100
        Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap() Andy Lutomirski <luto@amacapital.net> - 2016-01-27 06:50 +0100

#1317097 — [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap()

From: Matthew Wilcox <willy@linux.intel.com>

track_pfn_insert() overwrites the pgprot that is passed in with a value
based on the VMA's page_prot.  This is a problem for people trying to
do clever things with the new vm_insert_pfn_prot() as it will simply
overwrite the passed protection flags.  If we use the current value of
the pgprot as the base, then it will behave as people are expecting.

Also fix track_pfn_remap() in the same way.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 arch/x86/mm/pat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index f4ae536..04e2e71 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -943,7 +943,7 @@ int track_pfn_remap(struct vm_area_struct *vma, pgprot_t *prot,
 			return -EINVAL;
 	}
 
-	*prot = __pgprot((pgprot_val(vma->vm_page_prot) & (~_PAGE_CACHE_MASK)) |
+	*prot = __pgprot((pgprot_val(*prot) & (~_PAGE_CACHE_MASK)) |
 			 cachemode2protval(pcm));
 
 	return 0;
@@ -959,7 +959,7 @@ int track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
 
 	/* Set prot based on lookup */
 	pcm = lookup_memtype(pfn_t_to_phys(pfn));
-	*prot = __pgprot((pgprot_val(vma->vm_page_prot) & (~_PAGE_CACHE_MASK)) |
+	*prot = __pgprot((pgprot_val(*prot) & (~_PAGE_CACHE_MASK)) |
 			 cachemode2protval(pcm));
 
 	return 0;
-- 
2.7.0.rc3

[toc] | [next] | [standalone]

#1317114

On Mon, Jan 25, 2016 at 9:25 AM, Matthew Wilcox
<matthew.r.wilcox@intel.com> wrote:
> From: Matthew Wilcox <willy@linux.intel.com>
>
> track_pfn_insert() overwrites the pgprot that is passed in with a value
> based on the VMA's page_prot.  This is a problem for people trying to
> do clever things with the new vm_insert_pfn_prot() as it will simply
> overwrite the passed protection flags.  If we use the current value of
> the pgprot as the base, then it will behave as people are expecting.
>
> Also fix track_pfn_remap() in the same way.

Well that's embarrassing.  Presumably it worked for me because I only
overrode the cacheability bits and lookup_memtype did the right thing.

But shouldn't the PAT code change the memtype if vm_insert_pfn_prot
requests it?  Or are there no callers that actually need that?  (HPET
doesn't, because there's a plain old ioremapped mapping.)

--Andy

[toc] | [prev] | [next] | [standalone]

#1317124

On Mon, Jan 25, 2016 at 9:33 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Jan 25, 2016 at 9:25 AM, Matthew Wilcox
> <matthew.r.wilcox@intel.com> wrote:
>> From: Matthew Wilcox <willy@linux.intel.com>
>>
>> track_pfn_insert() overwrites the pgprot that is passed in with a value
>> based on the VMA's page_prot.  This is a problem for people trying to
>> do clever things with the new vm_insert_pfn_prot() as it will simply
>> overwrite the passed protection flags.  If we use the current value of
>> the pgprot as the base, then it will behave as people are expecting.
>>
>> Also fix track_pfn_remap() in the same way.
>
> Well that's embarrassing.  Presumably it worked for me because I only
> overrode the cacheability bits and lookup_memtype did the right thing.
>
> But shouldn't the PAT code change the memtype if vm_insert_pfn_prot
> requests it?  Or are there no callers that actually need that?  (HPET
> doesn't, because there's a plain old ioremapped mapping.)
>

Looking a bit further, track_pfn_remap does this, while
track_pfn_insert does not.  I don't know why

I'm also a bit confused as to how any of this works.  There doesn't
seem to be any reference counting of memtypes, so I don't understand
why, say, remapping the same range twice and then freeing them in FIFO
order doesn't break the memtype code.  (There's VM_PAT, but that seems
likely to be extremely fragile.)

--Andy

[toc] | [prev] | [next] | [standalone]

#1318627 — Re: [PATCH 1/3] x86: Honour passed pgprot in track_pfn_insert() and track_pfn_remap()

On Mon, Jan 25, 2016 at 09:33:35AM -0800, Andy Lutomirski wrote:
> On Mon, Jan 25, 2016 at 9:25 AM, Matthew Wilcox
> <matthew.r.wilcox@intel.com> wrote:
> > From: Matthew Wilcox <willy@linux.intel.com>
> >
> > track_pfn_insert() overwrites the pgprot that is passed in with a value
> > based on the VMA's page_prot.  This is a problem for people trying to
> > do clever things with the new vm_insert_pfn_prot() as it will simply
> > overwrite the passed protection flags.  If we use the current value of
> > the pgprot as the base, then it will behave as people are expecting.
> >
> > Also fix track_pfn_remap() in the same way.
> 
> Well that's embarrassing.  Presumably it worked for me because I only
> overrode the cacheability bits and lookup_memtype did the right thing.
> 
> But shouldn't the PAT code change the memtype if vm_insert_pfn_prot
> requests it?  Or are there no callers that actually need that?  (HPET
> doesn't, because there's a plain old ioremapped mapping.)

I'm confused.  Here's what I understand:

 - on x86, the bits in pgprot can be considered as two sets of bits;
   the 'cacheability bits' -- those in _PAGE_CACHE_MASK and the
   'protection bits' -- PRESENT, RW, USER, ACCESSED, NX
 - The purpose of track_pfn_insert() is to ensure that the cacheability bits
   are the same on all mappings of a given page, as strongly advised by the
   Intel manuals [1].  So track_pfn_insert() is really only supposed to
   modify _PAGE_CACHE_MASK of the passed pgprot, but in fact it ends up
   modifying the protection bits as well, due to the bug.

I don't think you overrode the cacheability bits at all.  It looks to
me like your patch ends up mapping the HPET into userspace writable.

I don't think the vm_insert_pfn_prot() call gets to change the memtype.
For one, that page may already be mapped into a differet userspace using
the pre-existing memtype, and [1] continues to bite you.  Then there
may be outstanding kernel users of the page that's being mapped in.

So I think track_pfn_insert() is doing the right thing with respect to
the cacheability bits (overwrite the ones passed in), it's just doing
an unexpected thing with regard to the protection bits, which my patch
should fix.

[1] "The PAT allows any memory type to be specified in the page tables,
and therefore it is possible to have a single physical page mapped to
two or more different linear addresses, each with different memory
types. Intel does not support this practice because it may lead to
undefined operations that can result in a system failure. In particular,
a WC page must never be aliased to a cacheable page because WC writes
may not check the processor caches."

[toc] | [prev] | [next] | [standalone]

#1318676

On Tue, Jan 26, 2016 at 8:40 PM, Matthew Wilcox <willy@linux.intel.com> wrote:
> On Mon, Jan 25, 2016 at 09:33:35AM -0800, Andy Lutomirski wrote:
>> On Mon, Jan 25, 2016 at 9:25 AM, Matthew Wilcox
>> <matthew.r.wilcox@intel.com> wrote:
>> > From: Matthew Wilcox <willy@linux.intel.com>
>> >
>> > track_pfn_insert() overwrites the pgprot that is passed in with a value
>> > based on the VMA's page_prot.  This is a problem for people trying to
>> > do clever things with the new vm_insert_pfn_prot() as it will simply
>> > overwrite the passed protection flags.  If we use the current value of
>> > the pgprot as the base, then it will behave as people are expecting.
>> >
>> > Also fix track_pfn_remap() in the same way.
>>
>> Well that's embarrassing.  Presumably it worked for me because I only
>> overrode the cacheability bits and lookup_memtype did the right thing.
>>
>> But shouldn't the PAT code change the memtype if vm_insert_pfn_prot
>> requests it?  Or are there no callers that actually need that?  (HPET
>> doesn't, because there's a plain old ioremapped mapping.)
>
> I'm confused.  Here's what I understand:
>
>  - on x86, the bits in pgprot can be considered as two sets of bits;
>    the 'cacheability bits' -- those in _PAGE_CACHE_MASK and the
>    'protection bits' -- PRESENT, RW, USER, ACCESSED, NX
>  - The purpose of track_pfn_insert() is to ensure that the cacheability bits
>    are the same on all mappings of a given page, as strongly advised by the
>    Intel manuals [1].  So track_pfn_insert() is really only supposed to
>    modify _PAGE_CACHE_MASK of the passed pgprot, but in fact it ends up
>    modifying the protection bits as well, due to the bug.
>
> I don't think you overrode the cacheability bits at all.  It looks to
> me like your patch ends up mapping the HPET into userspace writable.

I sure hope not.  If vm_page_prot was writable, something was already
broken, because this is the vvar mapping, and the vvar mapping is
VM_READ (and not even VM_MAYREAD).

>
> I don't think the vm_insert_pfn_prot() call gets to change the memtype.
> For one, that page may already be mapped into a differet userspace using
> the pre-existing memtype, and [1] continues to bite you.  Then there
> may be outstanding kernel users of the page that's being mapped in.

So why was remap_pfn_range different?  I'm sure there was a reason.

I don't think that whatever_pfn_prot should ever map a page
inconsistently, but I find it surprising that some of the variants
call reserve_memtype to change the memtype and others don't.

Anyway, this is in no way an objection to your patches.

--Andy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web