Re: [Xen-devel] [PATCH 2/2] x86-64/Xen: eliminate W+X mappings

[Juergen Gross]

On 12/12/17 11:48, Jan Beulich wrote:
 On 12.12.17 at 11:38,  wrote:
>> * Jan Beulich  wrote:
>>> --- 4.15-rc3/arch/x86/xen/mmu_pv.c
>>> +++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/mmu_pv.c
>>> @@ -1902,6 +1902,16 @@ void __init xen_setup_kernel_pagetable(p
>>> /* Graft it onto L4[511][510] */
>>> copy_page(level2_kernel_pgt, l2);
>>>  
>>> +   /* Zap execute permission from the ident map. Due to the sharing of
>>> +* L1 entries we need to do this in the L2. */
>>
>> please use the customary (multi-line) comment style:
>>
>>   /*
>>* Comment .
>>* .. goes here.
>>*/
>>
>> specified in Documentation/CodingStyle.
> 
> I would have but didn't because all other comments in this function
> use this (wrong) style. I've concluded that consistency is better
> here than matching the style doc. If the Xen maintainers tell me
> otherwise, I'll happily adjust the patch.

Yes, please use the correct style with new comments.

> 
>>> +   if (__supported_pte_mask & _PAGE_NX)
>>> +   for (i = 0; i < PTRS_PER_PMD; ++i) {
>>> +   if (pmd_none(level2_ident_pgt[i]))
>>> +   continue;
>>> +   level2_ident_pgt[i] =
>>> +   pmd_set_flags(level2_ident_pgt[i], _PAGE_NX);
>>
>> So the line break here is quite distracting, especially considering how 
>> similar it 
>> is to the alignment of the 'continue' statement. I.e. visually it looks like 
>> control flow alignment.
>>
>> Would be much better to just leave it a single page and ignore checkpatch 
>> here.
> 
> Again I'll wait to see what the Xen maintainers think. I too dislike
> line splits like this one, but the line ended up quite a bit too long,
> not just a character or two. I also wasn't sure whether splitting
> between the function arguments would be okay, leaving the first
> line just slightly too long.

That would result in a 80 character line, which IMHO is the best choice
here.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86-64/Xen: eliminate W+X mappings

[Ingo Molnar]

* Jan Beulich  wrote:

> >>> On 12.12.17 at 11:38,  wrote:
> > * Jan Beulich  wrote:
> >> --- 4.15-rc3/arch/x86/xen/mmu_pv.c
> >> +++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/mmu_pv.c
> >> @@ -1902,6 +1902,16 @@ void __init xen_setup_kernel_pagetable(p
> >>/* Graft it onto L4[511][510] */
> >>copy_page(level2_kernel_pgt, l2);
> >>  
> >> +  /* Zap execute permission from the ident map. Due to the sharing of
> >> +   * L1 entries we need to do this in the L2. */
> > 
> > please use the customary (multi-line) comment style:
> > 
> >   /*
> >* Comment .
> >* .. goes here.
> >*/
> > 
> > specified in Documentation/CodingStyle.
> 
> I would have but didn't because all other comments in this function
> use this (wrong) style. I've concluded that consistency is better
> here than matching the style doc. If the Xen maintainers tell me
> otherwise, I'll happily adjust the patch.

Then it should be cleaned up in a separate patch. The file is in arch/x86/ and 
both Documentation/CodingStyle and Linus's position is pretty unambiguous on 
this, 
there's no special exceptions for ugliness in arch/x86/ as far as I'm concerned.

Please guys fix this mess, NAK otherwise.

Thanks,

Ingo

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86-64/Xen: eliminate W+X mappings

[Jan Beulich]

>>> On 12.12.17 at 11:38,  wrote:
> * Jan Beulich  wrote:
>> --- 4.15-rc3/arch/x86/xen/mmu_pv.c
>> +++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/mmu_pv.c
>> @@ -1902,6 +1902,16 @@ void __init xen_setup_kernel_pagetable(p
>>  /* Graft it onto L4[511][510] */
>>  copy_page(level2_kernel_pgt, l2);
>>  
>> +/* Zap execute permission from the ident map. Due to the sharing of
>> + * L1 entries we need to do this in the L2. */
> 
> please use the customary (multi-line) comment style:
> 
>   /*
>* Comment .
>* .. goes here.
>*/
> 
> specified in Documentation/CodingStyle.

I would have but didn't because all other comments in this function
use this (wrong) style. I've concluded that consistency is better
here than matching the style doc. If the Xen maintainers tell me
otherwise, I'll happily adjust the patch.

>> +if (__supported_pte_mask & _PAGE_NX)
>> +for (i = 0; i < PTRS_PER_PMD; ++i) {
>> +if (pmd_none(level2_ident_pgt[i]))
>> +continue;
>> +level2_ident_pgt[i] =
>> +pmd_set_flags(level2_ident_pgt[i], _PAGE_NX);
> 
> So the line break here is quite distracting, especially considering how 
> similar it 
> is to the alignment of the 'continue' statement. I.e. visually it looks like 
> control flow alignment.
> 
> Would be much better to just leave it a single page and ignore checkpatch 
> here.

Again I'll wait to see what the Xen maintainers think. I too dislike
line splits like this one, but the line ended up quite a bit too long,
not just a character or two. I also wasn't sure whether splitting
between the function arguments would be okay, leaving the first
line just slightly too long.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86-64/Xen: eliminate W+X mappings

[Ingo Molnar]

* Jan Beulich  wrote:

> A few thousand such pages are usually left around due to the re-use of
> L1 tables having been provided by the hypervisor (Dom0) or tool stack
> (DomU). Set NX in the direct map variant, which needs to be done in L2
> due to the dual use of the re-used L1s.
> 
> For x86_configure_nx() to actually do what it is supposed to do, call
> get_cpu_cap() first. This was broken by commit 4763ed4d45 ("x86, mm:
> Clean up and simplify NX enablement") when switching away from the
> direct EFER read.
> 
> Signed-off-by: Jan Beulich 
> ---
> While I certainly dislike the added header inclusion to obtain the
> prototype for get_cpu_cap(), I couldn't find a better alternative. I'm
> open to suggestions.
> ---
>  arch/x86/xen/enlighten_pv.c |3 +++
>  arch/x86/xen/mmu_pv.c   |   10 ++
>  2 files changed, 13 insertions(+)
> 
> --- 4.15-rc3/arch/x86/xen/enlighten_pv.c
> +++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/enlighten_pv.c
> @@ -88,6 +88,8 @@
>  #include "multicalls.h"
>  #include "pmu.h"
>  
> +#include "../kernel/cpu/cpu.h" /* get_cpu_cap() */
> +
>  void *xen_initial_gdt;
>  
>  static int xen_cpu_up_prepare_pv(unsigned int cpu);
> @@ -1258,6 +1260,7 @@ asmlinkage __visible void __init xen_sta
>   __userpte_alloc_gfp &= ~__GFP_HIGHMEM;
>  
>   /* Work out if we support NX */
> + get_cpu_cap(_cpu_data);
>   x86_configure_nx();
>  
>   /* Get mfn list */
> --- 4.15-rc3/arch/x86/xen/mmu_pv.c
> +++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/mmu_pv.c
> @@ -1902,6 +1902,16 @@ void __init xen_setup_kernel_pagetable(p
>   /* Graft it onto L4[511][510] */
>   copy_page(level2_kernel_pgt, l2);
>  
> + /* Zap execute permission from the ident map. Due to the sharing of
> +  * L1 entries we need to do this in the L2. */

please use the customary (multi-line) comment style:

  /*
   * Comment .
   * .. goes here.
   */

specified in Documentation/CodingStyle.

> + if (__supported_pte_mask & _PAGE_NX)
> + for (i = 0; i < PTRS_PER_PMD; ++i) {
> + if (pmd_none(level2_ident_pgt[i]))
> + continue;
> + level2_ident_pgt[i] =
> + pmd_set_flags(level2_ident_pgt[i], _PAGE_NX);

So the line break here is quite distracting, especially considering how similar 
it 
is to the alignment of the 'continue' statement. I.e. visually it looks like 
control flow alignment.

Would be much better to just leave it a single page and ignore checkpatch here.

Thanks,

Ingo

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] x86-64/Xen: eliminate W+X mappings

[Jan Beulich]

A few thousand such pages are usually left around due to the re-use of
L1 tables having been provided by the hypervisor (Dom0) or tool stack
(DomU). Set NX in the direct map variant, which needs to be done in L2
due to the dual use of the re-used L1s.

For x86_configure_nx() to actually do what it is supposed to do, call
get_cpu_cap() first. This was broken by commit 4763ed4d45 ("x86, mm:
Clean up and simplify NX enablement") when switching away from the
direct EFER read.

Signed-off-by: Jan Beulich 
---
While I certainly dislike the added header inclusion to obtain the
prototype for get_cpu_cap(), I couldn't find a better alternative. I'm
open to suggestions.
---
 arch/x86/xen/enlighten_pv.c |3 +++
 arch/x86/xen/mmu_pv.c   |   10 ++
 2 files changed, 13 insertions(+)

--- 4.15-rc3/arch/x86/xen/enlighten_pv.c
+++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/enlighten_pv.c
@@ -88,6 +88,8 @@
 #include "multicalls.h"
 #include "pmu.h"
 
+#include "../kernel/cpu/cpu.h" /* get_cpu_cap() */
+
 void *xen_initial_gdt;
 
 static int xen_cpu_up_prepare_pv(unsigned int cpu);
@@ -1258,6 +1260,7 @@ asmlinkage __visible void __init xen_sta
__userpte_alloc_gfp &= ~__GFP_HIGHMEM;
 
/* Work out if we support NX */
+   get_cpu_cap(_cpu_data);
x86_configure_nx();
 
/* Get mfn list */
--- 4.15-rc3/arch/x86/xen/mmu_pv.c
+++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/mmu_pv.c
@@ -1902,6 +1902,16 @@ void __init xen_setup_kernel_pagetable(p
/* Graft it onto L4[511][510] */
copy_page(level2_kernel_pgt, l2);
 
+   /* Zap execute permission from the ident map. Due to the sharing of
+* L1 entries we need to do this in the L2. */
+   if (__supported_pte_mask & _PAGE_NX)
+   for (i = 0; i < PTRS_PER_PMD; ++i) {
+   if (pmd_none(level2_ident_pgt[i]))
+   continue;
+   level2_ident_pgt[i] =
+   pmd_set_flags(level2_ident_pgt[i], _PAGE_NX);
+   }
+
/* Copy the initial P->M table mappings if necessary. */
i = pgd_index(xen_start_info->mfn_list);
if (i && i < pgd_index(__START_KERNEL_map))




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel