The later verions of the KAISER pathces (pre-PTI) allowed the user/kernel
shared areas to be GLOBAL. The thought was that this would reduce the
TLB overhead of keeping two copies of these mappings.
During the switch over to PTI, we seem to have lost our ability to have
GLOBAL mappings. This adds them back.
Did we have some reason for leaving out global pages entirely? We
_should_ have all the TLB flushing mechanics in place to handle these
already since all of our kernel mapping changes have to know how to
flush global pages regardless.
Cc: Andy Lutomirski <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: [email protected]
From: Dave Hansen <[email protected]>
Kernel mappings are historically _PAGE_GLOBAL. But, with PTI, we do not
want them to be _PAGE_GLOBAL. We currently accomplish this by simply
clearing _PAGE_GLOBAL from the suppotred mask which ensures it is
cleansed from many of our PTE construction sites:
if (!static_cpu_has(X86_FEATURE_PTI))
__supported_pte_mask |= _PAGE_GLOBAL;
But, this also means that we now get *no* opportunity to use global
pages with PTI, even for data which is shared such as the cpu_entry_area
and entry/exit text.
This patch introduces a new mask: __PAGE_KERNEL_GLOBAL. This mask
can be thought of as the default global bit value when creating kernel
mappings. We make it _PAGE_GLOBAL when PTI=n, but 0 when PTI=y. This
ensures that on PTI kernels, all of the __PAGE_KERNEL_* users will not
get _PAGE_GLOBAL.
This also restores _PAGE_GLOBAL to __supported_pte_mask, allowing it
to be set in the first place.
Signed-off-by: Dave Hansen <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: [email protected]
---
b/arch/x86/include/asm/pgtable_types.h | 9 ++++++++-
b/arch/x86/mm/init.c | 8 +-------
b/arch/x86/mm/pageattr.c | 9 +++++----
3 files changed, 14 insertions(+), 12 deletions(-)
diff -puN arch/x86/include/asm/pgtable_types.h~kpti-no-global-for-kernel-mappings arch/x86/include/asm/pgtable_types.h
--- a/arch/x86/include/asm/pgtable_types.h~kpti-no-global-for-kernel-mappings 2018-02-13 15:17:56.144210060 -0800
+++ b/arch/x86/include/asm/pgtable_types.h 2018-02-13 15:17:56.152210060 -0800
@@ -180,8 +180,15 @@ enum page_cache_mode {
#define PAGE_READONLY_EXEC __pgprot(_PAGE_PRESENT | _PAGE_USER | \
_PAGE_ACCESSED)
+#ifdef CONFIG_PAGE_TABLE_ISOLATION
+#define __PAGE_KERNEL_GLOBAL 0
+#else
+#define __PAGE_KERNEL_GLOBAL _PAGE_GLOBAL
+#endif
+
#define __PAGE_KERNEL_EXEC \
- (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
+ (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | \
+ __PAGE_KERNEL_GLOBAL)
#define __PAGE_KERNEL (__PAGE_KERNEL_EXEC | _PAGE_NX)
#define __PAGE_KERNEL_RO (__PAGE_KERNEL & ~_PAGE_RW)
diff -puN arch/x86/mm/init.c~kpti-no-global-for-kernel-mappings arch/x86/mm/init.c
--- a/arch/x86/mm/init.c~kpti-no-global-for-kernel-mappings 2018-02-13 15:17:56.146210060 -0800
+++ b/arch/x86/mm/init.c 2018-02-13 15:17:56.152210060 -0800
@@ -162,12 +162,6 @@ struct map_range {
static int page_size_mask;
-static void enable_global_pages(void)
-{
- if (!static_cpu_has(X86_FEATURE_PTI))
- __supported_pte_mask |= _PAGE_GLOBAL;
-}
-
static void __init probe_page_size_mask(void)
{
/*
@@ -189,7 +183,7 @@ static void __init probe_page_size_mask(
__supported_pte_mask &= ~_PAGE_GLOBAL;
if (boot_cpu_has(X86_FEATURE_PGE)) {
cr4_set_bits_and_update_boot(X86_CR4_PGE);
- enable_global_pages();
+ __supported_pte_mask |= _PAGE_GLOBAL;
}
/* Enable 1 GB linear kernel mappings if available: */
diff -puN arch/x86/mm/pageattr.c~kpti-no-global-for-kernel-mappings arch/x86/mm/pageattr.c
--- a/arch/x86/mm/pageattr.c~kpti-no-global-for-kernel-mappings 2018-02-13 15:17:56.148210060 -0800
+++ b/arch/x86/mm/pageattr.c 2018-02-13 15:17:56.153210060 -0800
@@ -593,7 +593,8 @@ try_preserve_large_page(pte_t *kpte, uns
* different bit positions in the two formats.
*/
req_prot = pgprot_4k_2_large(req_prot);
- req_prot = pgprot_set_on_present(req_prot, _PAGE_GLOBAL | _PAGE_PSE);
+ req_prot = pgprot_set_on_present(req_prot,
+ __PAGE_KERNEL_GLOBAL | _PAGE_PSE);
req_prot = canon_pgprot(req_prot);
/*
@@ -703,7 +704,7 @@ __split_large_page(struct cpa_data *cpa,
return 1;
}
- ref_prot = pgprot_set_on_present(ref_prot, _PAGE_GLOBAL);
+ ref_prot = pgprot_set_on_present(ref_prot, __PAGE_KERNEL_GLOBAL);
/*
* Get the target pfn from the original entry:
@@ -926,7 +927,7 @@ static void populate_pte(struct cpa_data
pte = pte_offset_kernel(pmd, start);
- pgprot = pgprot_set_on_present(pgprot, _PAGE_GLOBAL);
+ pgprot = pgprot_set_on_present(pgprot, __PAGE_KERNEL_GLOBAL);
pgprot = canon_pgprot(pgprot);
while (num_pages-- && start < end) {
@@ -1219,7 +1220,7 @@ repeat:
new_prot = static_protections(new_prot, address, pfn);
- new_prot = pgprot_set_on_present(new_prot, _PAGE_GLOBAL);
+ new_prot = pgprot_set_on_present(new_prot, __PAGE_KERNEL_GLOBAL);
/*
* We need to keep the pfn from the existing PTE,
_
From: Dave Hansen <[email protected]>
The entry/exit text and cpu_entry_area are mapped into userspace and
the kernel. But, they are not _PAGE_GLOBAL. This creates unnecessary
TLB misses.
Add the _PAGE_GLOBAL flag for these areas.
After this patch, we can see that "GLB" shows up in each copy of the
page tables, that we have the same number of global entries in each
and that they are the *same* entries.
# grep -c GLB /sys/kernel/debug/page_tables/*
/sys/kernel/debug/page_tables/current_kernel:11
/sys/kernel/debug/page_tables/current_user:11
/sys/kernel/debug/page_tables/kernel:11
# for f in `ls /sys/kernel/debug/page_tables/`; do grep GLB /sys/kernel/debug/page_tables/$f > $f.GLB; done
# md5sum *.GLB
9caae8ad6a1fb53aca2407ec037f612d current_kernel.GLB
9caae8ad6a1fb53aca2407ec037f612d current_user.GLB
9caae8ad6a1fb53aca2407ec037f612d kernel.GLB
A quick visual audit also shows that all the entries make sense.
0xfffffe0000000000 is the cpu_entry_area and 0xffffffff81c00000
is the entry/exit text:
# grep -c GLB /sys/kernel/debug/page_tables/current_user
0xfffffe0000000000-0xfffffe0000002000 8K ro GLB NX pte
0xfffffe0000002000-0xfffffe0000003000 4K RW GLB NX pte
0xfffffe0000003000-0xfffffe0000006000 12K ro GLB NX pte
0xfffffe0000006000-0xfffffe0000007000 4K ro GLB x pte
0xfffffe0000007000-0xfffffe000000d000 24K RW GLB NX pte
0xfffffe000002d000-0xfffffe000002e000 4K ro GLB NX pte
0xfffffe000002e000-0xfffffe000002f000 4K RW GLB NX pte
0xfffffe000002f000-0xfffffe0000032000 12K ro GLB NX pte
0xfffffe0000032000-0xfffffe0000033000 4K ro GLB x pte
0xfffffe0000033000-0xfffffe0000039000 24K RW GLB NX pte
0xffffffff81c00000-0xffffffff81e00000 2M ro PSE GLB x pmd
Signed-off-by: Dave Hansen <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: [email protected]
---
b/arch/x86/mm/cpu_entry_area.c | 7 +++++++
b/arch/x86/mm/pti.c | 9 ++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff -puN arch/x86/mm/cpu_entry_area.c~kpti-why-no-global arch/x86/mm/cpu_entry_area.c
--- a/arch/x86/mm/cpu_entry_area.c~kpti-why-no-global 2018-02-13 15:17:56.735210059 -0800
+++ b/arch/x86/mm/cpu_entry_area.c 2018-02-13 15:17:56.740210059 -0800
@@ -28,6 +28,13 @@ void cea_set_pte(void *cea_vaddr, phys_a
{
unsigned long va = (unsigned long) cea_vaddr;
+ /*
+ * The cpu_entry_area is shared between the user and kernel
+ * page tables. All of its ptes can safely be global.
+ */
+ if (boot_cpu_has(X86_FEATURE_PGE))
+ pgprot_val(flags) |= _PAGE_GLOBAL;
+
set_pte_vaddr(va, pfn_pte(pa >> PAGE_SHIFT, flags));
}
diff -puN arch/x86/mm/pti.c~kpti-why-no-global arch/x86/mm/pti.c
--- a/arch/x86/mm/pti.c~kpti-why-no-global 2018-02-13 15:17:56.737210059 -0800
+++ b/arch/x86/mm/pti.c 2018-02-13 15:17:56.740210059 -0800
@@ -300,6 +300,13 @@ pti_clone_pmds(unsigned long start, unsi
return;
/*
+ * Setting 'target_pmd' below creates a mapping in both
+ * the user and kernel page tables. It is effectively
+ * global, so set it as global in both copies.
+ */
+ *pmd = pmd_set_flags(*pmd, _PAGE_GLOBAL);
+
+ /*
* Copy the PMD. That is, the kernelmode and usermode
* tables will share the last-level page tables of this
* address range
@@ -348,7 +355,7 @@ static void __init pti_clone_entry_text(
{
pti_clone_pmds((unsigned long) __entry_text_start,
(unsigned long) __irqentry_text_end,
- _PAGE_RW | _PAGE_GLOBAL);
+ _PAGE_RW);
}
/*
_
From: Dave Hansen <[email protected]>
The pageattr code has a pattern repeated where it sets a PTE bit
for present PTEs but clears it for non-present PTEs. This helps
to keep pte_none() from getting messed up. _PAGE_GLOBAL is the
most frequent target of this pattern.
This pattern also includes a nice, copy-and-pasted comment.
I want to do some special stuff with _PAGE_GLOBAL in a moment,
so refactor this a _bit_ to centralize the comment and the
bit operations.
Signed-off-by: Dave Hansen <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: [email protected]
---
b/arch/x86/mm/pageattr.c | 65 ++++++++++++++---------------------------------
1 file changed, 20 insertions(+), 45 deletions(-)
diff -puN arch/x86/mm/pageattr.c~kpti-centralize-global-setting arch/x86/mm/pageattr.c
--- a/arch/x86/mm/pageattr.c~kpti-centralize-global-setting 2018-02-13 15:17:55.602210062 -0800
+++ b/arch/x86/mm/pageattr.c 2018-02-13 15:17:55.606210062 -0800
@@ -512,6 +512,22 @@ static void __set_pmd_pte(pte_t *kpte, u
#endif
}
+static pgprot_t pgprot_set_on_present(pgprot_t prot, pteval_t flags)
+{
+ /*
+ * Set 'flags' only if PRESENT. Ensures that we do not
+ * set flags in an otherwise empty PTE breaking pte_none().
+ * A later function (such as canon_pgprot()) must clear
+ * possibly unsupported flags (like _PAGE_GLOBAL).
+ */
+ if (pgprot_val(prot) & _PAGE_PRESENT)
+ pgprot_val(prot) |= flags;
+ else
+ pgprot_val(prot) &= ~flags;
+
+ return prot;
+}
+
static int
try_preserve_large_page(pte_t *kpte, unsigned long address,
struct cpa_data *cpa)
@@ -577,18 +593,7 @@ try_preserve_large_page(pte_t *kpte, uns
* different bit positions in the two formats.
*/
req_prot = pgprot_4k_2_large(req_prot);
-
- /*
- * Set the PSE and GLOBAL flags only if the PRESENT flag is
- * set otherwise pmd_present/pmd_huge will return true even on
- * a non present pmd. The canon_pgprot will clear _PAGE_GLOBAL
- * for the ancient hardware that doesn't support it.
- */
- if (pgprot_val(req_prot) & _PAGE_PRESENT)
- pgprot_val(req_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
- else
- pgprot_val(req_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
-
+ req_prot = pgprot_set_on_present(req_prot, _PAGE_GLOBAL | _PAGE_PSE);
req_prot = canon_pgprot(req_prot);
/*
@@ -698,16 +703,7 @@ __split_large_page(struct cpa_data *cpa,
return 1;
}
- /*
- * Set the GLOBAL flags only if the PRESENT flag is set
- * otherwise pmd/pte_present will return true even on a non
- * present pmd/pte. The canon_pgprot will clear _PAGE_GLOBAL
- * for the ancient hardware that doesn't support it.
- */
- if (pgprot_val(ref_prot) & _PAGE_PRESENT)
- pgprot_val(ref_prot) |= _PAGE_GLOBAL;
- else
- pgprot_val(ref_prot) &= ~_PAGE_GLOBAL;
+ ref_prot = pgprot_set_on_present(ref_prot, _PAGE_GLOBAL);
/*
* Get the target pfn from the original entry:
@@ -930,18 +926,7 @@ static void populate_pte(struct cpa_data
pte = pte_offset_kernel(pmd, start);
- /*
- * Set the GLOBAL flags only if the PRESENT flag is
- * set otherwise pte_present will return true even on
- * a non present pte. The canon_pgprot will clear
- * _PAGE_GLOBAL for the ancient hardware that doesn't
- * support it.
- */
- if (pgprot_val(pgprot) & _PAGE_PRESENT)
- pgprot_val(pgprot) |= _PAGE_GLOBAL;
- else
- pgprot_val(pgprot) &= ~_PAGE_GLOBAL;
-
+ pgprot = pgprot_set_on_present(pgprot, _PAGE_GLOBAL);
pgprot = canon_pgprot(pgprot);
while (num_pages-- && start < end) {
@@ -1234,17 +1219,7 @@ repeat:
new_prot = static_protections(new_prot, address, pfn);
- /*
- * Set the GLOBAL flags only if the PRESENT flag is
- * set otherwise pte_present will return true even on
- * a non present pte. The canon_pgprot will clear
- * _PAGE_GLOBAL for the ancient hardware that doesn't
- * support it.
- */
- if (pgprot_val(new_prot) & _PAGE_PRESENT)
- pgprot_val(new_prot) |= _PAGE_GLOBAL;
- else
- pgprot_val(new_prot) &= ~_PAGE_GLOBAL;
+ new_prot = pgprot_set_on_present(new_prot, _PAGE_GLOBAL);
/*
* We need to keep the pfn from the existing PTE,
_
On Thu, Feb 15, 2018 at 5:20 AM, Dave Hansen
<[email protected]> wrote:
>
> During the switch over to PTI, we seem to have lost our ability to have
> GLOBAL mappings.
Oops. Odd, I have this distinct memory of somebody even _testing_ the
global bit performance when I pointed out that we shouldn't just make
the bit go away entirely.
[ goes back and looks at archives ]
Oh, that was in fact you who did that performance test.
Heh. Anyway, back then you claimed a noticeable improvement on that
will-it-scale test (although a bigger one when pcid wasn't available),
so yes, if we lost the "global pages for the shared user/kernel
mapping" bit we should definitely get this fixed.
Did you perhaps re-run any benchmark numbers just to verify? Because
it's always good to back up patches that should improve performance
with actual numbers..
Linus
On 02/15/2018 09:47 AM, Linus Torvalds wrote:
> On Thu, Feb 15, 2018 at 5:20 AM, Dave Hansen
> <[email protected]> wrote:
>>
>> During the switch over to PTI, we seem to have lost our ability to have
>> GLOBAL mappings.
>
> Oops. Odd, I have this distinct memory of somebody even _testing_ the
> global bit performance when I pointed out that we shouldn't just make
> the bit go away entirely.
>
> [ goes back and looks at archives ]
>
> Oh, that was in fact you who did that performance test.
...
> Did you perhaps re-run any benchmark numbers just to verify? Because
> it's always good to back up patches that should improve performance
> with actual numbers..
Nope, haven't done it yet, but I will.
I wanted to double-check that there was not a reason for doing the
global disabling other than the K8 TLB mismatch issues that Thomas fixed
a few weeks ago:
> commit 52994c256df36fda9a715697431cba9daecb6b11
> Author: Thomas Gleixner <[email protected]>
> Date: Wed Jan 3 15:57:59 2018 +0100
>
> x86/pti: Make sure the user/kernel PTEs match
>
> Meelis reported that his K8 Athlon64 emits MCE warnings when PTI is
> enabled:
>
> [Hardware Error]: Error Addr: 0x0000ffff81e000e0
> [Hardware Error]: MC1 Error: L1 TLB multimatch.
> [Hardware Error]: cache level: L1, tx: INSN
On 02/15/2018 09:47 AM, Linus Torvalds wrote:
> On Thu, Feb 15, 2018 at 5:20 AM, Dave Hansen
> <[email protected]> wrote:
>> During the switch over to PTI, we seem to have lost our ability to have
>> GLOBAL mappings.
...
> Did you perhaps re-run any benchmark numbers just to verify? Because
> it's always good to back up patches that should improve performance
> with actual numbers..
Same test as last time except I'm using all 4 cores on a Skylake desktop
instead of just 1. The test is this:
> https://github.com/antonblanchard/will-it-scale/blob/master/tests/lseek1.c
With PCIDs, lseek()s/second go up around 2% to 3% with the these patches
enabling the global bit (it's noisy). I measured it at 3% before, so
definitely the same ballpark. That was also before all of Andy's
trampoline stuff and the syscall fast path removal.
Dave Hansen <[email protected]> wrote:
>
> From: Dave Hansen <[email protected]>
>
> Kernel mappings are historically _PAGE_GLOBAL. But, with PTI, we do not
> want them to be _PAGE_GLOBAL. We currently accomplish this by simply
> clearing _PAGE_GLOBAL from the suppotred mask which ensures it is
> cleansed from many of our PTE construction sites:
>
> if (!static_cpu_has(X86_FEATURE_PTI))
> __supported_pte_mask |= _PAGE_GLOBAL;
>
> But, this also means that we now get *no* opportunity to use global
> pages with PTI, even for data which is shared such as the cpu_entry_area
> and entry/exit text.
Doesn’t this patch change the kernel behavior when the “nopti” parameter is used?
Dave Hansen <[email protected]> wrote:
> On 02/16/2018 09:47 AM, Nadav Amit wrote:
>>> But, this also means that we now get *no* opportunity to use
>>> global pages with PTI, even for data which is shared such as the
>>> cpu_entry_area and entry/exit text.
>>
>> Doesn’t this patch change the kernel behavior when the “nopti”
>> parameter is used?
>
> I don't think so. It takes the "nopti" behavior and effectively makes
> it apply everywhere. So it changes the PTI behavior, not the "nopti"
> behavior.
>
> Maybe it would help to quote the code that you think does this instead
> of the description. :)
Sorry, I thought it is obvious - so I must be missing something.
> +#ifdef CONFIG_PAGE_TABLE_ISOLATION
> +#define __PAGE_KERNEL_GLOBAL 0
> +#else
> +#define __PAGE_KERNEL_GLOBAL _PAGE_GLOBAL
> +#endif
...
> --- a/arch/x86/mm/pageattr.c~kpti-no-global-for-kernel-mappings 2018-02-13 15:17:56.148210060 -0800
> +++ b/arch/x86/mm/pageattr.c 2018-02-13 15:17:56.153210060 -0800
> @@ -593,7 +593,8 @@ try_preserve_large_page(pte_t *kpte, uns
> * different bit positions in the two formats.
> */
> req_prot = pgprot_4k_2_large(req_prot);
> - req_prot = pgprot_set_on_present(req_prot, _PAGE_GLOBAL | _PAGE_PSE);
> + req_prot = pgprot_set_on_present(req_prot,
> + __PAGE_KERNEL_GLOBAL | _PAGE_PSE);
> req_prot = canon_pgprot(req_prot);
From these chunks, it seems to me as req_prot will not have the global bit
on when “nopti” parameter is provided. What am I missing?
On 02/16/2018 10:25 AM, Nadav Amit wrote:
>> +#ifdef CONFIG_PAGE_TABLE_ISOLATION
>> +#define __PAGE_KERNEL_GLOBAL 0
>> +#else
>> +#define __PAGE_KERNEL_GLOBAL _PAGE_GLOBAL
>> +#endif
> ...
>> --- a/arch/x86/mm/pageattr.c~kpti-no-global-for-kernel-mappings 2018-02-13 15:17:56.148210060 -0800
>> +++ b/arch/x86/mm/pageattr.c 2018-02-13 15:17:56.153210060 -0800
>> @@ -593,7 +593,8 @@ try_preserve_large_page(pte_t *kpte, uns
>> * different bit positions in the two formats.
>> */
>> req_prot = pgprot_4k_2_large(req_prot);
>> - req_prot = pgprot_set_on_present(req_prot, _PAGE_GLOBAL | _PAGE_PSE);
>> + req_prot = pgprot_set_on_present(req_prot,
>> + __PAGE_KERNEL_GLOBAL | _PAGE_PSE);
>> req_prot = canon_pgprot(req_prot);
> From these chunks, it seems to me as req_prot will not have the global bit
> on when “nopti” parameter is provided. What am I missing?
That's a good point. The current patch does not allow the use of
_PAGE_GLOBAL via _PAGE_KERNEL_GLOBAL when CONFIG_PAGE_TABLE_ISOLATION=y,
but booted with nopti. It's a simple enough fix. Logically:
#ifdef CONFIG_PAGE_TABLE_ISOLATION
#define __PAGE_KERNEL_GLOBAL static_cpu_has(X86_FEATURE_PTI) ?
0 : _PAGE_GLOBAL
#else
#define __PAGE_KERNEL_GLOBAL _PAGE_GLOBAL
#endif
But I don't really want to hide that gunk in a macro like that. It
might make more sense as a static inline. I'll give that a shot and resent.
On 02/16/2018 09:47 AM, Nadav Amit wrote:
>> But, this also means that we now get *no* opportunity to use
>> global pages with PTI, even for data which is shared such as the
>> cpu_entry_area and entry/exit text.
>
> Doesn’t this patch change the kernel behavior when the “nopti”
> parameter is used?
I don't think so. It takes the "nopti" behavior and effectively makes
it apply everywhere. So it changes the PTI behavior, not the "nopti"
behavior.
Maybe it would help to quote the code that you think does this instead
of the description. :)
Dave Hansen <[email protected]> wrote:
> On 02/16/2018 10:25 AM, Nadav Amit wrote:
>>> +#ifdef CONFIG_PAGE_TABLE_ISOLATION
>>> +#define __PAGE_KERNEL_GLOBAL 0
>>> +#else
>>> +#define __PAGE_KERNEL_GLOBAL _PAGE_GLOBAL
>>> +#endif
>> ...
>>> --- a/arch/x86/mm/pageattr.c~kpti-no-global-for-kernel-mappings 2018-02-13 15:17:56.148210060 -0800
>>> +++ b/arch/x86/mm/pageattr.c 2018-02-13 15:17:56.153210060 -0800
>>> @@ -593,7 +593,8 @@ try_preserve_large_page(pte_t *kpte, uns
>>> * different bit positions in the two formats.
>>> */
>>> req_prot = pgprot_4k_2_large(req_prot);
>>> - req_prot = pgprot_set_on_present(req_prot, _PAGE_GLOBAL | _PAGE_PSE);
>>> + req_prot = pgprot_set_on_present(req_prot,
>>> + __PAGE_KERNEL_GLOBAL | _PAGE_PSE);
>>> req_prot = canon_pgprot(req_prot);
>> From these chunks, it seems to me as req_prot will not have the global bit
>> on when “nopti” parameter is provided. What am I missing?
>
> That's a good point. The current patch does not allow the use of
> _PAGE_GLOBAL via _PAGE_KERNEL_GLOBAL when CONFIG_PAGE_TABLE_ISOLATION=y,
> but booted with nopti. It's a simple enough fix. Logically:
>
> #ifdef CONFIG_PAGE_TABLE_ISOLATION
> #define __PAGE_KERNEL_GLOBAL static_cpu_has(X86_FEATURE_PTI) ?
> 0 : _PAGE_GLOBAL
> #else
> #define __PAGE_KERNEL_GLOBAL _PAGE_GLOBAL
> #endif
>
> But I don't really want to hide that gunk in a macro like that. It
> might make more sense as a static inline. I'll give that a shot and resent.
Since determining whether PTI is on is done in several places in the kernel,
maybe there should a single function to determine whether PTI is on,
something like:
static inline bool is_pti_on(void)
{
return IS_ENABLED(CONFIG_PAGE_TABLE_ISOLATION) &&
static_cpu_has(X86_FEATURE_PTI);
}
On 02/16/2018 11:54 AM, Nadav Amit wrote:
>> But I don't really want to hide that gunk in a macro like that. It
>> might make more sense as a static inline. I'll give that a shot and resent.
> Since determining whether PTI is on is done in several places in the kernel,
> maybe there should a single function to determine whether PTI is on,
> something like:
>
> static inline bool is_pti_on(void)
> {
> return IS_ENABLED(CONFIG_PAGE_TABLE_ISOLATION) &&
> static_cpu_has(X86_FEATURE_PTI);
> }
We should be able to do it with disabled-features.h and the X86_FEATURE
bit. I'll look into it.
On 02/16/2018 10:25 AM, Nadav Amit wrote:
>> --- a/arch/x86/mm/pageattr.c~kpti-no-global-for-kernel-mappings 2018-02-13 15:17:56.148210060 -0800
>> +++ b/arch/x86/mm/pageattr.c 2018-02-13 15:17:56.153210060 -0800
>> @@ -593,7 +593,8 @@ try_preserve_large_page(pte_t *kpte, uns
>> * different bit positions in the two formats.
>> */
>> req_prot = pgprot_4k_2_large(req_prot);
>> - req_prot = pgprot_set_on_present(req_prot, _PAGE_GLOBAL | _PAGE_PSE);
>> + req_prot = pgprot_set_on_present(req_prot,
>> + __PAGE_KERNEL_GLOBAL | _PAGE_PSE);
>> req_prot = canon_pgprot(req_prot);
> From these chunks, it seems to me as req_prot will not have the global bit
> on when “nopti” parameter is provided. What am I missing?
BTW, this code is broken. It's trying to unconditionally set
_PAGE_GLOBAL whenever set do change_page_attr() and friends. It gets
fixed up by canon_pgprot(), but it's wrong to do in the first place.
I've got a better fix for this coming.