| Message ID | 20180508121638.174022-1-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/08/2018 05:16 AM, Alexander Potapenko wrote: > Similarly to commit 187e91fe5e91 > ("x86/boot/64/clang: Use fixup_pointer() to access 'next_early_pgt'"), > '__supported_pte_mask' must be also accessed using fixup_pointer() to > avoid position-dependent relocations. > > Signed-off-by: Alexander Potapenko <glider@google.com> > Fixes: fb43d6cb91ef ("x86/mm: Do not auto-massage page protections") In the interests of standalone changelogs, I'd really appreciate an actual explanation of what's going on here. Your patch makes the code uglier and doesn't fix anything functional from what I can see. The other commit has some explanation, so it seems like the rules for accessing globals in head64.c are different than other files because... something. The functional problem here is that it causes insta-reboots? Do we have anything we can do to keep us from recreating these kinds of regressions all the time?
On Tue, May 8, 2018 at 4:30 PM Dave Hansen <dave.hansen@linux.intel.com> wrote: > On 05/08/2018 05:16 AM, Alexander Potapenko wrote: > > Similarly to commit 187e91fe5e91 > > ("x86/boot/64/clang: Use fixup_pointer() to access 'next_early_pgt'"), > > '__supported_pte_mask' must be also accessed using fixup_pointer() to > > avoid position-dependent relocations. > > > > Signed-off-by: Alexander Potapenko <glider@google.com> > > Fixes: fb43d6cb91ef ("x86/mm: Do not auto-massage page protections") > In the interests of standalone changelogs, I'd really appreciate an > actual explanation of what's going on here. Your patch makes the code > uglier and doesn't fix anything functional from what I can see. You're right, sure. I'll send a patch with an updated description. > The other commit has some explanation, so it seems like the rules for > accessing globals in head64.c are different than other files because... > something. The problem as far as I understand it is that the code in __startup_64() can be relocated during execution, but the compiler doesn't have to generate PC-relative relocations when accessing globals from that function. > The functional problem here is that it causes insta-reboots? True. > Do we have anything we can do to keep us from recreating these kinds of > regressions all the time? I'm not really aware of the possible options in the kernel land. Looks like a task for some objtool-like utility? As long as these regressions are caught with Clang, setting up a 0day Clang builder might help. At least I should've added a comment regarding this to __startup_64() last time.
On 05/08/2018 07:50 AM, Alexander Potapenko wrote: >>> Similarly to commit 187e91fe5e91 >>> ("x86/boot/64/clang: Use fixup_pointer() to access 'next_early_pgt'"), >>> '__supported_pte_mask' must be also accessed using fixup_pointer() to >>> avoid position-dependent relocations. >>> >>> Signed-off-by: Alexander Potapenko <glider@google.com> >>> Fixes: fb43d6cb91ef ("x86/mm: Do not auto-massage page protections") > >> In the interests of standalone changelogs, I'd really appreciate an >> actual explanation of what's going on here. Your patch makes the code >> uglier and doesn't fix anything functional from what I can see. > You're right, sure. I'll send a patch with an updated description. Great, thanks! >> Do we have anything we can do to keep us from recreating these kinds of >> regressions all the time? > I'm not really aware of the possible options in the kernel land. Looks like > a task for some objtool-like utility? > As long as these regressions are caught with Clang, setting up a 0day Clang > builder might help. I've asked the 0day folks if this is doable. It would be great to see it added.
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c index 0c408f8c4ed4..1b36ae4d0035 100644 --- a/arch/x86/kernel/head64.c +++ b/arch/x86/kernel/head64.c @@ -113,6 +113,7 @@ unsigned long __head __startup_64(unsigned long physaddr, p4dval_t *p4d; pudval_t *pud; pmdval_t *pmd, pmd_entry; + pteval_t *mask_ptr; bool la57; int i; unsigned int *next_pgt_ptr; @@ -196,7 +197,8 @@ unsigned long __head __startup_64(unsigned long physaddr, pmd_entry = __PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL; /* Filter out unsupported __PAGE_KERNEL_* bits: */ - pmd_entry &= __supported_pte_mask; + mask_ptr = (pteval_t *)fixup_pointer(&__supported_pte_mask, physaddr); + pmd_entry &= *mask_ptr; pmd_entry += sme_get_me_mask(); pmd_entry += physaddr;
Similarly to commit 187e91fe5e91 ("x86/boot/64/clang: Use fixup_pointer() to access 'next_early_pgt'"), '__supported_pte_mask' must be also accessed using fixup_pointer() to avoid position-dependent relocations. Signed-off-by: Alexander Potapenko <glider@google.com> Fixes: fb43d6cb91ef ("x86/mm: Do not auto-massage page protections") --- arch/x86/kernel/head64.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)