[PATCH] x86/boot/64/clang: Use fixup_pointer() to access '__supported_pte_mask'

[PATCH] x86/boot/64/clang: Use fixup_pointer() to access '__supported_pte_mask'

[Alexander Potapenko]

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(-)

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;
 
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH] x86/boot/64/clang: Use fixup_pointer() to access '__supported_pte_mask'

[Dave Hansen]

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?

Re: [PATCH] x86/boot/64/clang: Use fixup_pointer() to access '__supported_pte_mask'

[Alexander Potapenko]

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.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH] x86/boot/64/clang: Use fixup_pointer() to access '__supported_pte_mask'

[Dave Hansen]

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.