* [PATCH for v4.17 0/2] Fix two crashes in decomression code
@ 2018-05-10 17:38 Kirill A. Shutemov
2018-05-10 17:38 ` [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() Kirill A. Shutemov
2018-05-10 17:38 ` [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory Kirill A. Shutemov
0 siblings, 2 replies; 8+ messages in thread
From: Kirill A. Shutemov @ 2018-05-10 17:38 UTC (permalink / raw)
To: Ingo Molnar, x86, Thomas Gleixner, H. Peter Anvin
Cc: Hugh Dickins, linux-kernel, Kirill A. Shutemov
Here's two fixes in early boot code that leads to instant reboot.
Kirill A. Shutemov (2):
x86/boot/compressed/64: Set up GOT for paging_prepare() and
cleanup_trampoline()
x86/boot/compressed/64: Fix moving page table out of trampoline memory
arch/x86/boot/compressed/head_64.S | 77 ++++++++++++++++++++++-----
arch/x86/boot/compressed/pgtable_64.c | 14 ++---
2 files changed, 67 insertions(+), 24 deletions(-)
--
2.17.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() 2018-05-10 17:38 [PATCH for v4.17 0/2] Fix two crashes in decomression code Kirill A. Shutemov @ 2018-05-10 17:38 ` Kirill A. Shutemov 2018-05-13 18:55 ` Thomas Gleixner 2018-05-10 17:38 ` [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory Kirill A. Shutemov 1 sibling, 1 reply; 8+ messages in thread From: Kirill A. Shutemov @ 2018-05-10 17:38 UTC (permalink / raw) To: Ingo Molnar, x86, Thomas Gleixner, H. Peter Anvin Cc: Hugh Dickins, linux-kernel, Kirill A. Shutemov Eric and Hugh have reported instant reboot due to my recent changes in decompression code. The root cause is that I didn't realize that we need to adjust GOT to be able to run C code that early. The problem is only visible with old toolchain. Binutils >= 2.24 is able to eliminate GOT references by replacing them with RIP-relative address loads[1]. We need to adjust GOT two times: - before calling paging_prepare() with address the binary was loaded by bootloader - before relocating the kernel to the new place with relocation address [1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=80d873266dec Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Fixes: 194a9749c73d ("x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G") Reported-by: Eric Dumazet <eric.dumazet@gmail.com> Reported-by: Hugh Dickins <hughd@google.com> --- arch/x86/boot/compressed/head_64.S | 66 ++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 13 deletions(-) diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index fca012baba19..6cbb2d64c91e 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -305,6 +305,21 @@ ENTRY(startup_64) /* Set up the stack */ leaq boot_stack_end(%rbx), %rsp + /* + * paging_prepare() and cleanup_trampoline() below can have GOT + * references. Adjust the table with address we are running at. + */ + + /* The GOP was not adjusted before */ + xorq %rax, %rax + + /* Calculate the address the binary is loaded at. */ + call 1f +1: popq %rdi + subq $1b, %rdi + + call adjust_gop + /* * At this point we are in long mode with 4-level paging enabled, * but we might want to enable 5-level paging or vice versa. @@ -381,6 +396,24 @@ trampoline_return: pushq $0 popfq + /* + * Previously we've adjusted the GOT with address the binary was + * loaded at. Now we need to re-adjust for relocation address. + */ + + /* + * Calculate the address the binary is loaded at. + * This address was used to adjust the table before and we need to + * undo the change. + */ + call 1f +1: popq %rax + subq $1b, %rax + + /* The new adjustment is relocation address */ + movq %rbx, %rdi + call adjust_gop + /* * Copy the compressed kernel to the end of our buffer * where decompression in place becomes safe. @@ -481,19 +514,6 @@ relocated: shrq $3, %rcx rep stosq -/* - * Adjust our own GOT - */ - leaq _got(%rip), %rdx - leaq _egot(%rip), %rcx -1: - cmpq %rcx, %rdx - jae 2f - addq %rbx, (%rdx) - addq $8, %rdx - jmp 1b -2: - /* * Do the extraction, and jump to the new kernel.. */ @@ -512,6 +532,26 @@ relocated: */ jmp *%rax +/* + * Adjust global offest table + * + * RAX is previous adjustment of the table to undo (0 if it's the first time we touch GOP). + * RDI is the new adjustment to apply. + */ +adjust_gop: + /* Walk through the GOT adding the address to the entries */ + leaq _got(%rip), %rdx + leaq _egot(%rip), %rcx +1: + cmpq %rcx, %rdx + jae 2f + subq %rax, (%rdx) /* Undo previous adjustment */ + addq %rdi, (%rdx) /* Apply the new adjustment */ + addq $8, %rdx + jmp 1b +2: + ret + .code32 /* * This is the 32-bit trampoline that will be copied over to low memory. -- 2.17.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() 2018-05-10 17:38 ` [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() Kirill A. Shutemov @ 2018-05-13 18:55 ` Thomas Gleixner 2018-05-13 20:03 ` Kirill A. Shutemov 0 siblings, 1 reply; 8+ messages in thread From: Thomas Gleixner @ 2018-05-13 18:55 UTC (permalink / raw) To: Kirill A. Shutemov Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel On Thu, 10 May 2018, Kirill A. Shutemov wrote: > + /* > + * paging_prepare() and cleanup_trampoline() below can have GOT > + * references. Adjust the table with address we are running at. > + */ > + > + /* The GOP was not adjusted before */ GOP == EFI speak for Graphics Output Protocol. What the heck? > + xorq %rax, %rax And this clearing of RAX is related to this because? Sure you need it for adjust_got() but adding a comment to that is too much asked for, right? > + /* Calculate the address the binary is loaded at. */ > + call 1f > +1: popq %rdi > + subq $1b, %rdi > + > + call adjust_gop > + > /* > * At this point we are in long mode with 4-level paging enabled, > * but we might want to enable 5-level paging or vice versa. > @@ -381,6 +396,24 @@ trampoline_return: > pushq $0 > popfq > > + /* > + * Previously we've adjusted the GOT with address the binary was > + * loaded at. Now we need to re-adjust for relocation address. > + */ Breaking up those comments makes it more readable, right? > + /* > + * Calculate the address the binary is loaded at. > + * This address was used to adjust the table before and we need to > + * undo the change. > + */ > + call 1f > +1: popq %rax > + subq $1b, %rax > + > + /* The new adjustment is relocation address */ is the relocation address > + movq %rbx, %rdi > + call adjust_gop > + > /* > * Copy the compressed kernel to the end of our buffer > * where decompression in place becomes safe. > @@ -481,19 +514,6 @@ relocated: > shrq $3, %rcx > rep stosq > > -/* > - * Adjust our own GOT > - */ > - leaq _got(%rip), %rdx > - leaq _egot(%rip), %rcx > -1: > - cmpq %rcx, %rdx > - jae 2f > - addq %rbx, (%rdx) > - addq $8, %rdx > - jmp 1b > -2: > - > /* > * Do the extraction, and jump to the new kernel.. > */ > @@ -512,6 +532,26 @@ relocated: > */ > jmp *%rax > > +/* > + * Adjust global offest table offest? > + * > + * RAX is previous adjustment of the table to undo (0 if it's the first time we touch GOP). is the previous And there is no reason to make that line overly long. > + * RDI is the new adjustment to apply. > + */ > +adjust_gop: > + /* Walk through the GOT adding the address to the entries */ > + leaq _got(%rip), %rdx > + leaq _egot(%rip), %rcx > +1: > + cmpq %rcx, %rdx > + jae 2f > + subq %rax, (%rdx) /* Undo previous adjustment */ > + addq %rdi, (%rdx) /* Apply the new adjustment */ > + addq $8, %rdx > + jmp 1b > +2: > + ret I'm really tired of your carelessness. The amount of half baken stuff you submit is way above the tolerance level by now. I asked you several times to be more careful, but you simply do not care at all. Get your act together finally. Thanks, tglx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() 2018-05-13 18:55 ` Thomas Gleixner @ 2018-05-13 20:03 ` Kirill A. Shutemov 2018-05-13 21:30 ` Thomas Gleixner 0 siblings, 1 reply; 8+ messages in thread From: Kirill A. Shutemov @ 2018-05-13 20:03 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel On Sun, May 13, 2018 at 06:55:46PM +0000, Thomas Gleixner wrote: > On Thu, 10 May 2018, Kirill A. Shutemov wrote: > > > + /* > > + * paging_prepare() and cleanup_trampoline() below can have GOT > > + * references. Adjust the table with address we are running at. > > + */ > > + > > + /* The GOP was not adjusted before */ > > GOP == EFI speak for Graphics Output Protocol. What the heck? I was not aware about Graphics Output Protocol. > > > + xorq %rax, %rax > > And this clearing of RAX is related to this because? Sure you need it for > adjust_got() but adding a comment to that is too much asked for, right? Huh? The comment just above the line describes why it's needed. > > + /* Calculate the address the binary is loaded at. */ > > + call 1f > > +1: popq %rdi > > + subq $1b, %rdi > > + > > + call adjust_gop > > + > > /* > > * At this point we are in long mode with 4-level paging enabled, > > * but we might want to enable 5-level paging or vice versa. > > @@ -381,6 +396,24 @@ trampoline_return: > > pushq $0 > > popfq > > > > + /* > > + * Previously we've adjusted the GOT with address the binary was > > + * loaded at. Now we need to re-adjust for relocation address. > > + */ > > Breaking up those comments makes it more readable, right? Yes, I think so. The first comment is for the whole block of code below. The second is the comment for the first step. > > + /* > > + * Calculate the address the binary is loaded at. > > + * This address was used to adjust the table before and we need to > > + * undo the change. > > + */ > > + call 1f > > +1: popq %rax > > + subq $1b, %rax > > + > > + /* The new adjustment is relocation address */ > > is the relocation address > > > + movq %rbx, %rdi > > + call adjust_gop > > + > > /* > > * Copy the compressed kernel to the end of our buffer > > * where decompression in place becomes safe. > > @@ -481,19 +514,6 @@ relocated: > > shrq $3, %rcx > > rep stosq > > > > -/* > > - * Adjust our own GOT > > - */ > > - leaq _got(%rip), %rdx > > - leaq _egot(%rip), %rcx > > -1: > > - cmpq %rcx, %rdx > > - jae 2f > > - addq %rbx, (%rdx) > > - addq $8, %rdx > > - jmp 1b > > -2: > > - > > /* > > * Do the extraction, and jump to the new kernel.. > > */ > > @@ -512,6 +532,26 @@ relocated: > > */ > > jmp *%rax > > > > +/* > > + * Adjust global offest table > > offest? > > > + * > > + * RAX is previous adjustment of the table to undo (0 if it's the first time we touch GOP). > > is the previous > > And there is no reason to make that line overly long. > > > + * RDI is the new adjustment to apply. > > + */ > > +adjust_gop: > > + /* Walk through the GOT adding the address to the entries */ > > + leaq _got(%rip), %rdx > > + leaq _egot(%rip), %rcx > > +1: > > + cmpq %rcx, %rdx > > + jae 2f > > + subq %rax, (%rdx) /* Undo previous adjustment */ > > + addq %rdi, (%rdx) /* Apply the new adjustment */ > > + addq $8, %rdx > > + jmp 1b > > +2: > > + ret > > I'm really tired of your carelessness. The amount of half baken stuff you > submit is way above the tolerance level by now. I asked you several times > to be more careful, but you simply do not care at all. Get your act > together finally. I don't think it's fair. Yes, I have hard time write correctly, even in my native languages. I'm blind to mistakes I do. I'm sorry about them. But I do care about bugs in my code and I do my best addressing them. It took a lot of effort to find root cause of the bug and your statement about my carelessness doesn't match my assessment. Have a nice day. -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() 2018-05-13 20:03 ` Kirill A. Shutemov @ 2018-05-13 21:30 ` Thomas Gleixner 0 siblings, 0 replies; 8+ messages in thread From: Thomas Gleixner @ 2018-05-13 21:30 UTC (permalink / raw) To: Kirill A. Shutemov Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel On Sun, 13 May 2018, Kirill A. Shutemov wrote: > On Sun, May 13, 2018 at 06:55:46PM +0000, Thomas Gleixner wrote: > > On Thu, 10 May 2018, Kirill A. Shutemov wrote: > > > > > + /* > > > + * paging_prepare() and cleanup_trampoline() below can have GOT > > > + * references. Adjust the table with address we are running at. > > > + */ > > > + > > > + /* The GOP was not adjusted before */ > > > > GOP == EFI speak for Graphics Output Protocol. What the heck? > > I was not aware about Graphics Output Protocol. That does not matter. GOP what ever you think it is has nothing to do here. This is about GOT or am I missing something? > > > + xorq %rax, %rax > > > > And this clearing of RAX is related to this because? Sure you need it for > > adjust_got() but adding a comment to that is too much asked for, right? > > Huh? The comment just above the line describes why it's needed. No it does not. It is a comment which has only value when you first read the comment above the function which is called 5 lines later. Comments should make sense on their own. To be honest I did not even make the connection when I read the function later. > > > + /* Calculate the address the binary is loaded at. */ > > > + call 1f > > > +1: popq %rdi > > > + subq $1b, %rdi > > > + > > > + call adjust_gop > > > + > > > /* > > > * At this point we are in long mode with 4-level paging enabled, > > > * but we might want to enable 5-level paging or vice versa. > > > @@ -381,6 +396,24 @@ trampoline_return: > > > pushq $0 > > > popfq > > > > > > + /* > > > + * Previously we've adjusted the GOT with address the binary was > > > + * loaded at. Now we need to re-adjust for relocation address. > > > + */ > > > > Breaking up those comments makes it more readable, right? > > Yes, I think so. > > The first comment is for the whole block of code below. The second is the > comment for the first step. Sorry no. It's just confusing as hell and a few weeks down the road it looks like somebody removed the code between the comments and forgot to update them. You can put an empty line into one comment block to separate paragraphs. > Yes, I have hard time write correctly, even in my native languages. > I'm blind to mistakes I do. I'm sorry about them. Sorry, that you have a problem with that, but you could have told me offlist long ago and we would have found a solution for this. Not knowing that, it just looks like being careless to the other side. Thanks, tglx ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory 2018-05-10 17:38 [PATCH for v4.17 0/2] Fix two crashes in decomression code Kirill A. Shutemov 2018-05-10 17:38 ` [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() Kirill A. Shutemov @ 2018-05-10 17:38 ` Kirill A. Shutemov 2018-05-13 18:56 ` Thomas Gleixner 1 sibling, 1 reply; 8+ messages in thread From: Kirill A. Shutemov @ 2018-05-10 17:38 UTC (permalink / raw) To: Ingo Molnar, x86, Thomas Gleixner, H. Peter Anvin Cc: Hugh Dickins, linux-kernel, Kirill A. Shutemov top_pgtable address has to be calculated relative to where the kernel image will be relocated for decompression, not relative to position of kernel is running at the moment. We do the same for the rest of page table we use the stage. It makes them safe from being overwritten during decompression. Calculate the address of top_pgtable in assembly and pass down to cleanup_trampoline(). Move the page table to .pgtable section where the rest of page tables are. The section is @nobits so we save 4k in kernel image. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Fixes: e9d0e6330eb8 ("x86/boot/compressed/64: Prepare new top-level page table for trampoline") --- arch/x86/boot/compressed/head_64.S | 11 +++++++++++ arch/x86/boot/compressed/pgtable_64.c | 14 +++----------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index 6cbb2d64c91e..1acf7b5d16cf 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -385,10 +385,14 @@ trampoline_return: /* * cleanup_trampoline() would restore trampoline memory. * + * RDI is address of the page table to use instead of page table + * in trampoline memory (if required). + * * RSI holds real mode data and needs to be preserved across * this function call. */ pushq %rsi + leaq top_pgtable(%rbx), %rdi call cleanup_trampoline popq %rsi @@ -689,3 +693,10 @@ boot_stack_end: .balign 4096 pgtable: .fill BOOT_PGT_SIZE, 1, 0 + +/* + * The page table is going to be used instead of page table in the trampoline + * memory. + */ +top_pgtable: + .fill PAGE_SIZE, 1, 0 diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c index 32af1cbcd903..a362fa0b849c 100644 --- a/arch/x86/boot/compressed/pgtable_64.c +++ b/arch/x86/boot/compressed/pgtable_64.c @@ -22,14 +22,6 @@ struct paging_config { /* Buffer to preserve trampoline memory */ static char trampoline_save[TRAMPOLINE_32BIT_SIZE]; -/* - * The page table is going to be used instead of page table in the trampoline - * memory. - * - * It must not be in BSS as BSS is cleared after cleanup_trampoline(). - */ -static char top_pgtable[PAGE_SIZE] __aligned(PAGE_SIZE) __section(.data); - /* * Trampoline address will be printed by extract_kernel() for debugging * purposes. @@ -134,7 +126,7 @@ struct paging_config paging_prepare(void) return paging_config; } -void cleanup_trampoline(void) +void cleanup_trampoline(void *pgtable) { void *trampoline_pgtable; @@ -145,8 +137,8 @@ void cleanup_trampoline(void) * if it's there. */ if ((void *)__native_read_cr3() == trampoline_pgtable) { - memcpy(top_pgtable, trampoline_pgtable, PAGE_SIZE); - native_write_cr3((unsigned long)top_pgtable); + memcpy(pgtable, trampoline_pgtable, PAGE_SIZE); + native_write_cr3((unsigned long)pgtable); } /* Restore trampoline memory */ -- 2.17.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory 2018-05-10 17:38 ` [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory Kirill A. Shutemov @ 2018-05-13 18:56 ` Thomas Gleixner 2018-05-13 20:05 ` Kirill A. Shutemov 0 siblings, 1 reply; 8+ messages in thread From: Thomas Gleixner @ 2018-05-13 18:56 UTC (permalink / raw) To: Kirill A. Shutemov Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel On Thu, 10 May 2018, Kirill A. Shutemov wrote: > top_pgtable address has to be calculated relative to where the kernel > image will be relocated for decompression, not relative to position of > kernel is running at the moment. We do the same for the rest of page > table we use the stage. It makes them safe from being overwritten during > decompression. > > Calculate the address of top_pgtable in assembly and pass down to > cleanup_trampoline(). > > Move the page table to .pgtable section where the rest of page tables > are. The section is @nobits so we save 4k in kernel image. So this is supposed to be a fix, but the whole changelog talks about WHAT the patch does and not WHY. Darn, we need proper description of the failure which is about to be fixed. It's not that hard and I'm really tired to tell you that over and over. > /* > * cleanup_trampoline() would restore trampoline memory. > * > + * RDI is address of the page table to use instead of page table > + * in trampoline memory (if required). Do you really believe that you understand that comment 6 month from now? Thanks, tglx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory 2018-05-13 18:56 ` Thomas Gleixner @ 2018-05-13 20:05 ` Kirill A. Shutemov 0 siblings, 0 replies; 8+ messages in thread From: Kirill A. Shutemov @ 2018-05-13 20:05 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel On Sun, May 13, 2018 at 06:56:14PM +0000, Thomas Gleixner wrote: > On Thu, 10 May 2018, Kirill A. Shutemov wrote: > > > top_pgtable address has to be calculated relative to where the kernel > > image will be relocated for decompression, not relative to position of > > kernel is running at the moment. We do the same for the rest of page > > table we use the stage. It makes them safe from being overwritten during > > decompression. > > > > Calculate the address of top_pgtable in assembly and pass down to > > cleanup_trampoline(). > > > > Move the page table to .pgtable section where the rest of page tables > > are. The section is @nobits so we save 4k in kernel image. > > So this is supposed to be a fix, but the whole changelog talks about WHAT > the patch does and not WHY. Darn, we need proper description of the failure > which is about to be fixed. "It makes them safe from being overwritten during decompression." > It's not that hard and I'm really tired to tell you that over and over. > > > /* > > * cleanup_trampoline() would restore trampoline memory. > > * > > + * RDI is address of the page table to use instead of page table > > + * in trampoline memory (if required). > > Do you really believe that you understand that comment 6 month from now? Yes, I think I will. -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-05-13 21:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-10 17:38 [PATCH for v4.17 0/2] Fix two crashes in decomression code Kirill A. Shutemov 2018-05-10 17:38 ` [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() Kirill A. Shutemov 2018-05-13 18:55 ` Thomas Gleixner 2018-05-13 20:03 ` Kirill A. Shutemov 2018-05-13 21:30 ` Thomas Gleixner 2018-05-10 17:38 ` [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory Kirill A. Shutemov 2018-05-13 18:56 ` Thomas Gleixner 2018-05-13 20:05 ` Kirill A. Shutemov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.