Re: [PATCH v1 1/3] arch/powerpc/set_memory: Implement set_memory_xx routines

[Balbir Singh]

On Fri, Aug 4, 2017 at 1:38 PM, Aneesh Kumar K.V
 wrote:
> Balbir Singh  writes:
>
>> On Wed, Aug 2, 2017 at 8:09 PM, Aneesh Kumar K.V
>>  wrote:
>>> Balbir Singh  writes:
>>>
 Add support for set_memory_xx routines. With the STRICT_KERNEL_RWX
 feature support we got support for changing the page permissions
 for pte ranges. This patch adds support for both radix and hash
 so that we can change their permissions via set/clear masks.

 A new helper is required for hash (hash__change_memory_range()
 is changed to hash__change_boot_memory_range() as it deals with
 bolted PTE's).

 hash__change_memory_range() works with vmalloc'ed PAGE_SIZE requests
 for permission changes. hash__change_memory_range() does not invoke
 updatepp, instead it changes the software PTE and invalidates the PTE.

 For radix, radix__change_memory_range() is setup to do the right
 thing for vmalloc'd addresses. It takes a new parameter to decide
 what attributes to set.

>>> 
>>>
 +int hash__change_memory_range(unsigned long start, unsigned long end,
 + unsigned long set, unsigned long clear)
 +{
 + unsigned long idx;
 + pgd_t *pgdp;
 + pud_t *pudp;
 + pmd_t *pmdp;
 + pte_t *ptep;
 +
 + start = ALIGN_DOWN(start, PAGE_SIZE);
 + end = PAGE_ALIGN(end); // aligns up
 +
 + /*
 +  * Update the software PTE and flush the entry.
 +  * This should cause a new fault with the right
 +  * things setup in the hash page table
 +  */
 + pr_debug("Changing flags on range %lx-%lx setting 0x%lx removing 
 0x%lx\n",
 +  start, end, set, clear);
 +
 + for (idx = start; idx < end; idx += PAGE_SIZE) {
>>>
>>>
 + pgdp = pgd_offset_k(idx);
 + pudp = pud_alloc(_mm, pgdp, idx);
 + if (!pudp)
 + return -1;
 + pmdp = pmd_alloc(_mm, pudp, idx);
 + if (!pmdp)
 + return -1;
 + ptep = pte_alloc_kernel(pmdp, idx);
 + if (!ptep)
 + return -1;
 + hash__pte_update(_mm, idx, ptep, clear, set, 0);
>>
>> I think this does the needful, if H_PAGE_HASHPTE is set, the flush
>> will happen
>>
 + hash__flush_tlb_kernel_range(idx, idx + PAGE_SIZE);
 + }
>>>
>>> You can use find_linux_pte_or_hugepte. with my recent patch series
>>> find_init_mm_pte() ?
>>>
>>
>> for pte_mkwrite and pte_wrprotect?
>
> For walking page table. I am not sure you really want to allocate page
> table in that function. If you do, then what will be the initial value
> of PTE ? We are requesting to set an clear from and existing PTE entry
> right ? If you find a none page table entry you should handle it via a
> fault ?
>

Fair enough, I've been lazy with that check, I'll fix it in v2

Balbir Singh.

Re: [PATCH v1 1/3] arch/powerpc/set_memory: Implement set_memory_xx routines

[Aneesh Kumar K.V]

Balbir Singh  writes:

> On Wed, Aug 2, 2017 at 8:09 PM, Aneesh Kumar K.V
>  wrote:
>> Balbir Singh  writes:
>>
>>> Add support for set_memory_xx routines. With the STRICT_KERNEL_RWX
>>> feature support we got support for changing the page permissions
>>> for pte ranges. This patch adds support for both radix and hash
>>> so that we can change their permissions via set/clear masks.
>>>
>>> A new helper is required for hash (hash__change_memory_range()
>>> is changed to hash__change_boot_memory_range() as it deals with
>>> bolted PTE's).
>>>
>>> hash__change_memory_range() works with vmalloc'ed PAGE_SIZE requests
>>> for permission changes. hash__change_memory_range() does not invoke
>>> updatepp, instead it changes the software PTE and invalidates the PTE.
>>>
>>> For radix, radix__change_memory_range() is setup to do the right
>>> thing for vmalloc'd addresses. It takes a new parameter to decide
>>> what attributes to set.
>>>
>> 
>>
>>> +int hash__change_memory_range(unsigned long start, unsigned long end,
>>> + unsigned long set, unsigned long clear)
>>> +{
>>> + unsigned long idx;
>>> + pgd_t *pgdp;
>>> + pud_t *pudp;
>>> + pmd_t *pmdp;
>>> + pte_t *ptep;
>>> +
>>> + start = ALIGN_DOWN(start, PAGE_SIZE);
>>> + end = PAGE_ALIGN(end); // aligns up
>>> +
>>> + /*
>>> +  * Update the software PTE and flush the entry.
>>> +  * This should cause a new fault with the right
>>> +  * things setup in the hash page table
>>> +  */
>>> + pr_debug("Changing flags on range %lx-%lx setting 0x%lx removing 
>>> 0x%lx\n",
>>> +  start, end, set, clear);
>>> +
>>> + for (idx = start; idx < end; idx += PAGE_SIZE) {
>>
>>
>>> + pgdp = pgd_offset_k(idx);
>>> + pudp = pud_alloc(_mm, pgdp, idx);
>>> + if (!pudp)
>>> + return -1;
>>> + pmdp = pmd_alloc(_mm, pudp, idx);
>>> + if (!pmdp)
>>> + return -1;
>>> + ptep = pte_alloc_kernel(pmdp, idx);
>>> + if (!ptep)
>>> + return -1;
>>> + hash__pte_update(_mm, idx, ptep, clear, set, 0);
>
> I think this does the needful, if H_PAGE_HASHPTE is set, the flush
> will happen
>
>>> + hash__flush_tlb_kernel_range(idx, idx + PAGE_SIZE);
>>> + }
>>
>> You can use find_linux_pte_or_hugepte. with my recent patch series
>> find_init_mm_pte() ?
>>
>
> for pte_mkwrite and pte_wrprotect?

For walking page table. I am not sure you really want to allocate page
table in that function. If you do, then what will be the initial value
of PTE ? We are requesting to set an clear from and existing PTE entry
right ? If you find a none page table entry you should handle it via a
fault ?


-aneesh

Re: [PATCH v1 1/3] arch/powerpc/set_memory: Implement set_memory_xx routines

[Balbir Singh]

On Wed, Aug 2, 2017 at 8:09 PM, Aneesh Kumar K.V
 wrote:
> Balbir Singh  writes:
>
>> Add support for set_memory_xx routines. With the STRICT_KERNEL_RWX
>> feature support we got support for changing the page permissions
>> for pte ranges. This patch adds support for both radix and hash
>> so that we can change their permissions via set/clear masks.
>>
>> A new helper is required for hash (hash__change_memory_range()
>> is changed to hash__change_boot_memory_range() as it deals with
>> bolted PTE's).
>>
>> hash__change_memory_range() works with vmalloc'ed PAGE_SIZE requests
>> for permission changes. hash__change_memory_range() does not invoke
>> updatepp, instead it changes the software PTE and invalidates the PTE.
>>
>> For radix, radix__change_memory_range() is setup to do the right
>> thing for vmalloc'd addresses. It takes a new parameter to decide
>> what attributes to set.
>>
> 
>
>> +int hash__change_memory_range(unsigned long start, unsigned long end,
>> + unsigned long set, unsigned long clear)
>> +{
>> + unsigned long idx;
>> + pgd_t *pgdp;
>> + pud_t *pudp;
>> + pmd_t *pmdp;
>> + pte_t *ptep;
>> +
>> + start = ALIGN_DOWN(start, PAGE_SIZE);
>> + end = PAGE_ALIGN(end); // aligns up
>> +
>> + /*
>> +  * Update the software PTE and flush the entry.
>> +  * This should cause a new fault with the right
>> +  * things setup in the hash page table
>> +  */
>> + pr_debug("Changing flags on range %lx-%lx setting 0x%lx removing 
>> 0x%lx\n",
>> +  start, end, set, clear);
>> +
>> + for (idx = start; idx < end; idx += PAGE_SIZE) {
>
>
>> + pgdp = pgd_offset_k(idx);
>> + pudp = pud_alloc(_mm, pgdp, idx);
>> + if (!pudp)
>> + return -1;
>> + pmdp = pmd_alloc(_mm, pudp, idx);
>> + if (!pmdp)
>> + return -1;
>> + ptep = pte_alloc_kernel(pmdp, idx);
>> + if (!ptep)
>> + return -1;
>> + hash__pte_update(_mm, idx, ptep, clear, set, 0);

I think this does the needful, if H_PAGE_HASHPTE is set, the flush
will happen

>> + hash__flush_tlb_kernel_range(idx, idx + PAGE_SIZE);
>> + }
>
> You can use find_linux_pte_or_hugepte. with my recent patch series
> find_init_mm_pte() ?
>

for pte_mkwrite and pte_wrprotect?

Balbir Singh.

Re: [PATCH v1 1/3] arch/powerpc/set_memory: Implement set_memory_xx routines

[Aneesh Kumar K.V]

Balbir Singh  writes:

> Add support for set_memory_xx routines. With the STRICT_KERNEL_RWX
> feature support we got support for changing the page permissions
> for pte ranges. This patch adds support for both radix and hash
> so that we can change their permissions via set/clear masks.
>
> A new helper is required for hash (hash__change_memory_range()
> is changed to hash__change_boot_memory_range() as it deals with
> bolted PTE's).
>
> hash__change_memory_range() works with vmalloc'ed PAGE_SIZE requests
> for permission changes. hash__change_memory_range() does not invoke
> updatepp, instead it changes the software PTE and invalidates the PTE.
>
> For radix, radix__change_memory_range() is setup to do the right
> thing for vmalloc'd addresses. It takes a new parameter to decide
> what attributes to set.
>


> +int hash__change_memory_range(unsigned long start, unsigned long end,
> + unsigned long set, unsigned long clear)
> +{
> + unsigned long idx;
> + pgd_t *pgdp;
> + pud_t *pudp;
> + pmd_t *pmdp;
> + pte_t *ptep;
> +
> + start = ALIGN_DOWN(start, PAGE_SIZE);
> + end = PAGE_ALIGN(end); // aligns up
> +
> + /*
> +  * Update the software PTE and flush the entry.
> +  * This should cause a new fault with the right
> +  * things setup in the hash page table
> +  */
> + pr_debug("Changing flags on range %lx-%lx setting 0x%lx removing 
> 0x%lx\n",
> +  start, end, set, clear);
> +
> + for (idx = start; idx < end; idx += PAGE_SIZE) {


> + pgdp = pgd_offset_k(idx);
> + pudp = pud_alloc(_mm, pgdp, idx);
> + if (!pudp)
> + return -1;
> + pmdp = pmd_alloc(_mm, pudp, idx);
> + if (!pmdp)
> + return -1;
> + ptep = pte_alloc_kernel(pmdp, idx);
> + if (!ptep)
> + return -1;
> + hash__pte_update(_mm, idx, ptep, clear, set, 0);
> + hash__flush_tlb_kernel_range(idx, idx + PAGE_SIZE);
> + }

You can use find_linux_pte_or_hugepte. with my recent patch series
find_init_mm_pte() ?

-aneesh

Re: [PATCH v1 1/3] arch/powerpc/set_memory: Implement set_memory_xx routines

[Balbir Singh]

On Tue, 1 Aug 2017 21:08:49 +0200
christophe leroy  wrote:

> Le 01/08/2017 à 13:25, Balbir Singh a écrit :
> > Add support for set_memory_xx routines. With the STRICT_KERNEL_RWX
> > feature support we got support for changing the page permissions
> > for pte ranges. This patch adds support for both radix and hash
> > so that we can change their permissions via set/clear masks.
> >
> > A new helper is required for hash (hash__change_memory_range()
> > is changed to hash__change_boot_memory_range() as it deals with
> > bolted PTE's).
> >
> > hash__change_memory_range() works with vmalloc'ed PAGE_SIZE requests
> > for permission changes. hash__change_memory_range() does not invoke
> > updatepp, instead it changes the software PTE and invalidates the PTE.
> >
> > For radix, radix__change_memory_range() is setup to do the right
> > thing for vmalloc'd addresses. It takes a new parameter to decide
> > what attributes to set.
> >
> > Signed-off-by: Balbir Singh 
> > ---
> >  arch/powerpc/include/asm/book3s/64/hash.h  |  6 +++
> >  arch/powerpc/include/asm/book3s/64/radix.h |  6 +++
> >  arch/powerpc/include/asm/set_memory.h  | 34 +++
> >  arch/powerpc/mm/pgtable-hash64.c   | 51 --
> >  arch/powerpc/mm/pgtable-radix.c| 26 ++--
> >  arch/powerpc/mm/pgtable_64.c   | 68 
> > ++
> >  6 files changed, 175 insertions(+), 16 deletions(-)
> >  create mode 100644 arch/powerpc/include/asm/set_memory.h
> >  
> 
> [...]
> 
> > diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
> > index 0736e94..3ee4c7d 100644
> > --- a/arch/powerpc/mm/pgtable_64.c
> > +++ b/arch/powerpc/mm/pgtable_64.c
> > @@ -514,3 +514,71 @@ void mark_initmem_nx(void)
> > hash__mark_initmem_nx();
> >  }
> >  #endif
> > +
> > +#ifdef CONFIG_ARCH_HAS_SET_MEMORY
> > +/*
> > + * Some of these bits are taken from arm64/mm/page_attr.c
> > + */
> > +static int change_memory_common(unsigned long addr, int numpages,
> > +   unsigned long set, unsigned long clear)
> > +{
> > +   unsigned long start = addr;
> > +   unsigned long size = PAGE_SIZE*numpages;
> > +   unsigned long end = start + size;
> > +   struct vm_struct *area;
> > +
> > +   if (!PAGE_ALIGNED(addr)) {
> > +   start &= PAGE_MASK;
> > +   end = start + size;
> > +   WARN_ON_ONCE(1);
> > +   }  
> 
> Why not just set start = addr & PAGE_MASK, then just do 
> WARN_ON_ONCE(start != addr), instead of that if ()

The code has been taken from arch/arm64/mm/page_attr.c. I did
not change any bits, but we could make changes.

> 
> > +
> > +   /*
> > +* So check whether the [addr, addr + size) interval is entirely
> > +* covered by precisely one VM area that has the VM_ALLOC flag set.
> > +*/
> > +   area = find_vm_area((void *)addr);
> > +   if (!area ||
> > +   end > (unsigned long)area->addr + area->size ||
> > +   !(area->flags & VM_ALLOC))
> > +   return -EINVAL;
> > +
> > +   if (!numpages)
> > +   return 0;  
> 
> Shouldn't that be tested earlier ?
> 

Same as above

> > +
> > +   if (radix_enabled())
> > +   return radix__change_memory_range(start, start + size,
> > +   set, clear);
> > +   else
> > +   return hash__change_memory_range(start, start + size,
> > +   set, clear);
> > +}  
> 
> The following functions should go in a place common to PPC32 and PPC64, 
> otherwise they will have to be duplicated when implementing for PPC32.
> Maybe the above function should also go in a common place, only the last 
> part should remain in a PPC64 dedicated part. It could be called 
> change_memory_range(), something like
> 
> int change_memory_range(unsigned long start, unsigned long end,
>   unsigned long set, unsigned long clear)
> {
>   if (radix_enabled())
>   return radix__change_memory_range(start, end,
> set, clear);
>   return hash__change_memory_range(start, end, set, clear);
> }
> 
> Then change_memory_range() could also be implemented for PPC32 later.

I was hoping that when we implement support for PPC32, we
could refactor the code then and move it to arch/powerpc/mm/page_attr.c
if required. What do you think?

> 
> > +
> > +int set_memory_ro(unsigned long addr, int numpages)
> > +{
> > +   return change_memory_common(addr, numpages,
> > +   0, _PAGE_WRITE);
> > +}
> > +EXPORT_SYMBOL(set_memory_ro);  
> 
> Take care that _PAGE_WRITE has value 0 when _PAGE_RO instead of _PAGE_RW 
> is defined (eg for the 8xx).
> 
> It would be better to use accessors like pte_wrprotect() and pte_mkwrite()
>

Sure we can definitely refactor this for PPC32, pte_wrprotect()
and pte_mkwrite() would require us to make the 

Re: [PATCH v1 1/3] arch/powerpc/set_memory: Implement set_memory_xx routines

[christophe leroy]

Le 01/08/2017 à 13:25, Balbir Singh a écrit :

Add support for set_memory_xx routines. With the STRICT_KERNEL_RWX
feature support we got support for changing the page permissions
for pte ranges. This patch adds support for both radix and hash
so that we can change their permissions via set/clear masks.

A new helper is required for hash (hash__change_memory_range()
is changed to hash__change_boot_memory_range() as it deals with
bolted PTE's).

hash__change_memory_range() works with vmalloc'ed PAGE_SIZE requests
for permission changes. hash__change_memory_range() does not invoke
updatepp, instead it changes the software PTE and invalidates the PTE.

For radix, radix__change_memory_range() is setup to do the right
thing for vmalloc'd addresses. It takes a new parameter to decide
what attributes to set.

Signed-off-by: Balbir Singh 
---
 arch/powerpc/include/asm/book3s/64/hash.h  |  6 +++
 arch/powerpc/include/asm/book3s/64/radix.h |  6 +++
 arch/powerpc/include/asm/set_memory.h  | 34 +++
 arch/powerpc/mm/pgtable-hash64.c   | 51 --
 arch/powerpc/mm/pgtable-radix.c| 26 ++--
 arch/powerpc/mm/pgtable_64.c   | 68 ++
 6 files changed, 175 insertions(+), 16 deletions(-)
 create mode 100644 arch/powerpc/include/asm/set_memory.h



[...]


diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
index 0736e94..3ee4c7d 100644
--- a/arch/powerpc/mm/pgtable_64.c
+++ b/arch/powerpc/mm/pgtable_64.c
@@ -514,3 +514,71 @@ void mark_initmem_nx(void)
hash__mark_initmem_nx();
 }
 #endif
+
+#ifdef CONFIG_ARCH_HAS_SET_MEMORY
+/*
+ * Some of these bits are taken from arm64/mm/page_attr.c
+ */
+static int change_memory_common(unsigned long addr, int numpages,
+   unsigned long set, unsigned long clear)
+{
+   unsigned long start = addr;
+   unsigned long size = PAGE_SIZE*numpages;
+   unsigned long end = start + size;
+   struct vm_struct *area;
+
+   if (!PAGE_ALIGNED(addr)) {
+   start &= PAGE_MASK;
+   end = start + size;
+   WARN_ON_ONCE(1);
+   }


Why not just set start = addr & PAGE_MASK, then just do 
WARN_ON_ONCE(start != addr), instead of that if ()



+
+   /*
+* So check whether the [addr, addr + size) interval is entirely
+* covered by precisely one VM area that has the VM_ALLOC flag set.
+*/
+   area = find_vm_area((void *)addr);
+   if (!area ||
+   end > (unsigned long)area->addr + area->size ||
+   !(area->flags & VM_ALLOC))
+   return -EINVAL;
+
+   if (!numpages)
+   return 0;


Shouldn't that be tested earlier ?


+
+   if (radix_enabled())
+   return radix__change_memory_range(start, start + size,
+   set, clear);
+   else
+   return hash__change_memory_range(start, start + size,
+   set, clear);
+}


The following functions should go in a place common to PPC32 and PPC64, 
otherwise they will have to be duplicated when implementing for PPC32.
Maybe the above function should also go in a common place, only the last 
part should remain in a PPC64 dedicated part. It could be called 
change_memory_range(), something like


int change_memory_range(unsigned long start, unsigned long end,
unsigned long set, unsigned long clear)
{
if (radix_enabled())
return radix__change_memory_range(start, end,
  set, clear);
return hash__change_memory_range(start, end, set, clear);
}

Then change_memory_range() could also be implemented for PPC32 later.


+
+int set_memory_ro(unsigned long addr, int numpages)
+{
+   return change_memory_common(addr, numpages,
+   0, _PAGE_WRITE);
+}
+EXPORT_SYMBOL(set_memory_ro);


Take care that _PAGE_WRITE has value 0 when _PAGE_RO instead of _PAGE_RW 
is defined (eg for the 8xx).


It would be better to use accessors like pte_wrprotect() and pte_mkwrite()


+
+int set_memory_rw(unsigned long addr, int numpages)
+{
+   return change_memory_common(addr, numpages,
+   _PAGE_WRITE, 0);
+}
+EXPORT_SYMBOL(set_memory_rw);
+
+int set_memory_nx(unsigned long addr, int numpages)
+{
+   return change_memory_common(addr, numpages,
+   0, _PAGE_EXEC);
+}
+EXPORT_SYMBOL(set_memory_nx);
+
+int set_memory_x(unsigned long addr, int numpages)
+{
+   return change_memory_common(addr, numpages,
+   _PAGE_EXEC, 0);
+}
+EXPORT_SYMBOL(set_memory_x);
+#endif



Christophe

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel 
antivirus Avast.