x86/boot/64/clang: Use fixup_pointer() to access '__supported_pte_mask'
diff mbox

Message ID 20180508121638.174022-1-glider@google.com
State New
Headers show

Commit Message

Alexander Potapenko May 8, 2018, 12:16 p.m. UTC
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(-)

Comments

Dave Hansen May 8, 2018, 2:30 p.m. UTC | #1
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?
Alexander Potapenko May 8, 2018, 2:50 p.m. UTC | #2
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.
Dave Hansen May 8, 2018, 4:25 p.m. UTC | #3
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.

Patch
diff mbox

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;