mprotect() resets caching policy

mprotect() resets caching policy

[Thomas Hellström]

Hi!

I'm working on an infrastructure to allow drm clients to flip arbitrary 
pages in and out of the AGP aperture (or any similar device). In order 
to avoid conflicting mappings for those pages, the caching attribute of 
both the kernel mapping and all VMA's is changed when binding / unbinding.

However, I noticed that mprotect() will, when run on a non-cached VMA, 
reset the caching policy. The line in mm/mprotect.c causing this problem is

newprot = protection_map[newflags & 0xf];

So a user could potentially run mprotect() and create a conflicting 
mapping which presumably is bad for stability on some architectures.

Since mprotect() only deals with rwx protection. I figure replacing the 
above with something like

newprot = (vm_page_prot & ~MPROT_MASK) | (protection_map[newflags & 0xf] 
& MPROT_MASK)

Where MPROT_MASK is a arch-dependent mask identifying the bits available 
to mprotect().

Alternatively, is there a way to disable mprotect() for a VMA?

Finally, is there a chance to get protection_map[] exported to modules?

Any comments would be appreciated. Please CC me since I'm not on the list.

Regards,
Thomas Hellström

Re: mprotect() resets caching policy

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2880 bytes --]

On Fri, 27 Jan 2006, Thomas Hellström wrote:
> 
> I'm working on an infrastructure to allow drm clients to flip arbitrary pages
> in and out of the AGP aperture (or any similar device). In order to avoid
> conflicting mappings for those pages, the caching attribute of both the kernel
> mapping and all VMA's is changed when binding / unbinding.

I can't comment on that plan.

> However, I noticed that mprotect() will, when run on a non-cached VMA, reset
> the caching policy. The line in mm/mprotect.c causing this problem is
> 
> newprot = protection_map[newflags & 0xf];
> 
> So a user could potentially run mprotect() and create a conflicting mapping
> which presumably is bad for stability on some architectures.

Perhaps: I think it already depends on what the architecture does.
newprot is used (a) to set vm_page_prot and (b) for use in pte_modify
(which change_protection applies to each pte present).

Now I think vm_page_prot is irrelevant to the kinds of VMA you are
interested in?  It's essential to provide the permissions/protections
when faulting a new page in, but your VMAs are fully managed by the
driver, and have all ptes already in place?  So wouldn't use vm_page_prot.

And pte_modify is implemented per-architecture: looking just at the
i386 implementation, yes, _PAGE_CHG_MASK looks like it'll mask off
the bits you understandably want it to retain.

> Since mprotect() only deals with rwx protection. I figure replacing the above
> with something like
> 
> newprot = (vm_page_prot & ~MPROT_MASK) | (protection_map[newflags & 0xf] &
> MPROT_MASK)
> 
> Where MPROT_MASK is a arch-dependent mask identifying the bits available to
> mprotect().

I think it's the per-architecture implementations of pte_modify that
you need to adjust.

It might be nice, but probably irrelevant, to have vm_page_prot maintained
in a similar way.  Whether every arch can do that with a straightforward
MPROT_MASK or _PAGE_CHG_MASK is not obvious to me: would more likely
need a pte_modify-like macro to do it.

But would that even be correct?  The same vm_page_prot would be applied
also to anonymous COWed pages from the mapping.  Very exceptional in the
case that interests you; but perhaps it's simpler to keep vm_page_prot
just for the rwx part of it.

> Alternatively, is there a way to disable mprotect() for a VMA?

Not at present.  It would be easier to add a VM_flag for that,
than to correct every architecture's pte_modify.  I'm not sure
whether "easier" amounts to "better" here.

> Finally, is there a chance to get protection_map[] exported to modules?

I very much doubt that.  Exporting a functional interface to it would
be preferable; but even that, I think the core mm would strongly resist
- the less of the pte business we export the better.  What were you
wanting it exported for?

Hugh

Re: mprotect() resets caching policy

[Thomas Hellström]

Hugh Dickins wrote:

>>However, I noticed that mprotect() will, when run on a non-cached VMA, reset
>>the caching policy. The line in mm/mprotect.c causing this problem is
>>
>>newprot = protection_map[newflags & 0xf];
>>
>>So a user could potentially run mprotect() and create a conflicting mapping
>>which presumably is bad for stability on some architectures.
>>    
>>
>
>Perhaps: I think it already depends on what the architecture does.
>newprot is used (a) to set vm_page_prot and (b) for use in pte_modify
>(which change_protection applies to each pte present).
>
>Now I think vm_page_prot is irrelevant to the kinds of VMA you are
>interested in?  It's essential to provide the permissions/protections
>when faulting a new page in, but your VMAs are fully managed by the
>driver, and have all ptes already in place?  So wouldn't use vm_page_prot.
>
>  
>
The strategy I use for changing the protection in currently a bit ugly, 
since it
1) Flushes caches.
2) Invalidates all relevant PTE entries
3) Changes the kernel mapping caching policy.
4) Flushes tlb
5) Relies on nopage() to get called to map the page with the new caching 
policy.
Here I must set vm_page_prot, since that it what is used for the newly 
mapped page.

>And pte_modify is implemented per-architecture: looking just at the
>i386 implementation, yes, _PAGE_CHG_MASK looks like it'll mask off
>the bits you understandably want it to retain.
>  
>
Yes. I've tried running mprotect() and it does indeed reset the caching 
policy.

>>Since mprotect() only deals with rwx protection. I figure replacing the above
>>with something like
>>
>>newprot = (vm_page_prot & ~MPROT_MASK) | (protection_map[newflags & 0xf] &
>>MPROT_MASK)
>>
>>Where MPROT_MASK is a arch-dependent mask identifying the bits available to
>>mprotect().
>>    
>>
>
>I think it's the per-architecture implementations of pte_modify that
>you need to adjust.
>  
>
Agreed. That would work unless also nopage() uses pte_modify to set the 
original caching policy. Then it will get masked away.

>It might be nice, but probably irrelevant, to have vm_page_prot maintained
>in a similar way.  Whether every arch can do that with a straightforward
>MPROT_MASK or _PAGE_CHG_MASK is not obvious to me: would more likely
>need a pte_modify-like macro to do it.
>
>But would that even be correct?  The same vm_page_prot would be applied
>also to anonymous COWed pages from the mapping.  Very exceptional in the
>case that interests you; but perhaps it's simpler to keep vm_page_prot
>just for the rwx part of it.
>  
>
I'm not sure it will work in my case, since the caching policy is a 
per-page attribute and I rely totally on nopage() to get called for 
every new page attached to a vma mapping the region.

>  
>
>>Alternatively, is there a way to disable mprotect() for a VMA?
>>    
>>
>
>Not at present.  It would be easier to add a VM_flag for that,
>than to correct every architecture's pte_modify.  I'm not sure
>whether "easier" amounts to "better" here.
>
>  
>
>>Finally, is there a chance to get protection_map[] exported to modules?
>>    
>>
>
>I very much doubt that.  Exporting a functional interface to it would
>be preferable; but even that, I think the core mm would strongly resist
>- the less of the pte business we export the better.  What were you
>wanting it exported for?
>
>  
>
When nopage() gets called, I set vm_page_prot to 

modify_for_caching( protection_map[vma-> vm_flags & 0x0f])

Again, since someone might have called mprotect() to change vm_flags and 
I can't rely on previous values of vm_page_prot since it's changing all 
the time.

Anyway, thanks for looking at this and if there is something I can do 
(patch maybe) to
correct this behaviour plese let me know.

/Thomas


>Hugh
>