[PATCH v2 0/3] x86: fix brk area initialization

[PATCH v2 0/3] x86: fix brk area initialization

[Juergen Gross]

The brk area needs to be zeroed initially, like the .bss section.
At the same time its memory should be covered by the ELF program
headers.

Juergen Gross (3):
  x86/xen: use clear_bss() for Xen PV guests
  x86: fix setup of brk area
  x86: fix .brk attribute in linker script

 arch/x86/include/asm/setup.h  |  3 +++
 arch/x86/kernel/head64.c      |  4 +++-
 arch/x86/kernel/vmlinux.lds.S |  2 +-
 arch/x86/xen/enlighten_pv.c   |  8 ++++++--
 arch/x86/xen/xen-head.S       | 10 +---------
 5 files changed, 14 insertions(+), 13 deletions(-)

-- 
2.35.3

[PATCH v2 1/3] x86/xen: use clear_bss() for Xen PV guests

[Juergen Gross]

Instead of clearing the bss area in assembly code, use the clear_bss()
function.

This requires to pass the start_info address as parameter to
xen_start_kernel() in order to avoid the xen_start_info being zeroed
again.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 arch/x86/include/asm/setup.h |  3 +++
 arch/x86/kernel/head64.c     |  2 +-
 arch/x86/xen/enlighten_pv.c  |  8 ++++++--
 arch/x86/xen/xen-head.S      | 10 +---------
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
index f8b9ee97a891..f37cbff7354c 100644
--- a/arch/x86/include/asm/setup.h
+++ b/arch/x86/include/asm/setup.h
@@ -120,6 +120,9 @@ void *extend_brk(size_t size, size_t align);
 	static char __brk_##name[size]
 
 extern void probe_roms(void);
+
+void clear_bss(void);
+
 #ifdef __i386__
 
 asmlinkage void __init i386_start_kernel(void);
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index bd4a34100ed0..e7e233209a8c 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -426,7 +426,7 @@ void __init do_early_exception(struct pt_regs *regs, int trapnr)
 
 /* Don't add a printk in there. printk relies on the PDA which is not initialized 
    yet. */
-static void __init clear_bss(void)
+void __init clear_bss(void)
 {
 	memset(__bss_start, 0,
 	       (unsigned long) __bss_stop - (unsigned long) __bss_start);
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index e3297b15701c..70fb2ea85e90 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1183,15 +1183,19 @@ static void __init xen_domu_set_legacy_features(void)
 extern void early_xen_iret_patch(void);
 
 /* First C function to be called on Xen boot */
-asmlinkage __visible void __init xen_start_kernel(void)
+asmlinkage __visible void __init xen_start_kernel(struct start_info *si)
 {
 	struct physdev_set_iopl set_iopl;
 	unsigned long initrd_start = 0;
 	int rc;
 
-	if (!xen_start_info)
+	if (!si)
 		return;
 
+	clear_bss();
+
+	xen_start_info = si;
+
 	__text_gen_insn(&early_xen_iret_patch,
 			JMP32_INSN_OPCODE, &early_xen_iret_patch, &xen_iret,
 			JMP32_INSN_SIZE);
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 3a2cd93bf059..13af6fe453e3 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -48,15 +48,6 @@ SYM_CODE_START(startup_xen)
 	ANNOTATE_NOENDBR
 	cld
 
-	/* Clear .bss */
-	xor %eax,%eax
-	mov $__bss_start, %rdi
-	mov $__bss_stop, %rcx
-	sub %rdi, %rcx
-	shr $3, %rcx
-	rep stosq
-
-	mov %rsi, xen_start_info
 	mov initial_stack(%rip), %rsp
 
 	/* Set up %gs.
@@ -71,6 +62,7 @@ SYM_CODE_START(startup_xen)
 	cdq
 	wrmsr
 
+	mov	%rsi, %rdi
 	call xen_start_kernel
 SYM_CODE_END(startup_xen)
 	__FINIT
-- 
2.35.3

Re: [PATCH v2 1/3] x86/xen: use clear_bss() for Xen PV guests

[Jan Beulich]

On 23.06.2022 11:46, Juergen Gross wrote:
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -1183,15 +1183,19 @@ static void __init xen_domu_set_legacy_features(void)
>  extern void early_xen_iret_patch(void);
>  
>  /* First C function to be called on Xen boot */
> -asmlinkage __visible void __init xen_start_kernel(void)
> +asmlinkage __visible void __init xen_start_kernel(struct start_info *si)
>  {
>  	struct physdev_set_iopl set_iopl;
>  	unsigned long initrd_start = 0;
>  	int rc;
>  
> -	if (!xen_start_info)
> +	if (!si)
>  		return;
>  
> +	clear_bss();

As per subsequent observation, this shouldn't really be needed: The
hypervisor (or tool stack for DomU-s) already does so. While I guess
we want to keep it to be on the safe side, maybe worth a comment?

Jan

Re: [PATCH v2 1/3] x86/xen: use clear_bss() for Xen PV guests

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1049 bytes --]

On 23.06.22 11:51, Jan Beulich wrote:
> On 23.06.2022 11:46, Juergen Gross wrote:
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -1183,15 +1183,19 @@ static void __init xen_domu_set_legacy_features(void)
>>   extern void early_xen_iret_patch(void);
>>   
>>   /* First C function to be called on Xen boot */
>> -asmlinkage __visible void __init xen_start_kernel(void)
>> +asmlinkage __visible void __init xen_start_kernel(struct start_info *si)
>>   {
>>   	struct physdev_set_iopl set_iopl;
>>   	unsigned long initrd_start = 0;
>>   	int rc;
>>   
>> -	if (!xen_start_info)
>> +	if (!si)
>>   		return;
>>   
>> +	clear_bss();
> 
> As per subsequent observation, this shouldn't really be needed: The
> hypervisor (or tool stack for DomU-s) already does so. While I guess
> we want to keep it to be on the safe side, maybe worth a comment?

Are you sure all possible boot loaders are clearing alloc-only sections?

I'd rather not count on e.g. grub doing this in all cases.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH v2 2/3] x86: fix setup of brk area

[Juergen Gross]

Commit e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
put the brk area into the .bss..brk section (placed directly behind
.bss), causing it not to be cleared initially. As the brk area is used
to allocate early page tables, these might contain garbage in not
explicitly written entries.

This is especially a problem for Xen PV guests, as the hypervisor will
validate page tables (check for writable page tables and hypervisor
private bits) before accepting them to be used. There have been reports
of early crashes of PV guests due to illegal page table contents.

Fix that by letting clear_bss() clear the brk area, too.

Fixes: e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/kernel/head64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index e7e233209a8c..6a3cfaf6b72a 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -430,6 +430,8 @@ void __init clear_bss(void)
 {
 	memset(__bss_start, 0,
 	       (unsigned long) __bss_stop - (unsigned long) __bss_start);
+	memset(__brk_base, 0,
+	       (unsigned long) __brk_limit - (unsigned long) __brk_base);
 }
 
 static unsigned long get_cmd_line_ptr(void)
-- 
2.35.3

Re: [PATCH v2 2/3] x86: fix setup of brk area

[Josh Poimboeuf]

Hi Juergen,

It helps to actually Cc the person who broke it ;-)

On Thu, Jun 23, 2022 at 11:46:07AM +0200, Juergen Gross wrote:
> Commit e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
> put the brk area into the .bss..brk section (placed directly behind
> .bss),

Hm? It didn't actually do that.

For individual translation units, it did rename the section from
".brk_reservation" to ".bss..brk".  But then during linking it's still
placed in .brk in vmlinux, just like before.

> causing it not to be cleared initially. As the brk area is used
> to allocate early page tables, these might contain garbage in not
> explicitly written entries.
> 
> This is especially a problem for Xen PV guests, as the hypervisor will
> validate page tables (check for writable page tables and hypervisor
> private bits) before accepting them to be used. There have been reports
> of early crashes of PV guests due to illegal page table contents.
> 
> Fix that by letting clear_bss() clear the brk area, too.

While it does make sense to clear the brk area, I don't understand how
my patch broke this.  How was it getting cleared before?

-- 
Josh

Re: [PATCH v2 2/3] x86: fix setup of brk area

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1828 bytes --]

On 29.06.22 19:14, Josh Poimboeuf wrote:
> Hi Juergen,
> 
> It helps to actually Cc the person who broke it ;-)
> 
> On Thu, Jun 23, 2022 at 11:46:07AM +0200, Juergen Gross wrote:
>> Commit e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
>> put the brk area into the .bss..brk section (placed directly behind
>> .bss),
> 
> Hm? It didn't actually do that.
> 
> For individual translation units, it did rename the section from
> ".brk_reservation" to ".bss..brk".  But then during linking it's still
> placed in .brk in vmlinux, just like before.

Sorry, I misread the patch commit message and was fooled by the fact that
bisection clearly pointed at this patch to have introduced the problem.

I only discovered later that the main issue was the added "NOLOAD"
attribute.

>> causing it not to be cleared initially. As the brk area is used
>> to allocate early page tables, these might contain garbage in not
>> explicitly written entries.
>>
>> This is especially a problem for Xen PV guests, as the hypervisor will
>> validate page tables (check for writable page tables and hypervisor
>> private bits) before accepting them to be used. There have been reports
>> of early crashes of PV guests due to illegal page table contents.
>>
>> Fix that by letting clear_bss() clear the brk area, too.
> 
> While it does make sense to clear the brk area, I don't understand how
> my patch broke this.  How was it getting cleared before?

It seemed to have worked by chance. The Xen hypervisor is clearing all
alloc-only sections when loading a kernel (this will "fix" the dom0
case reliably together with patch 3 of this series).

Grub might do the clearing, too (for the PV domU case), but I haven't
verified that by code inspection.

I'll drop the "Fixes:" tag.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH v2 3/3] x86: fix .brk attribute in linker script

[Juergen Gross]

Commit e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
added the "NOLOAD" attribute to the .brk section as a "failsafe"
measure.

Unfortunately this leads to the linker no longer covering the .brk
section in a program header, resulting in the kernel loader not knowing
that the memory for the .brk section must be reserved.

This has led to crashes when loading the kernel as PV dom0 under Xen,
but other scenarios could be hit by the same problem (e.g. in case an
uncompressed kernel is used and the initrd is placed directly behind
it).

So drop the "NOLOAD" attribute. This has been verified to correctly
cover the .brk section by a program header of the resulting ELF file.

Fixes: e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 81aba718ecd5..9487ce8c13ee 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -385,7 +385,7 @@ SECTIONS
 	__end_of_kernel_reserve = .;
 
 	. = ALIGN(PAGE_SIZE);
-	.brk (NOLOAD) : AT(ADDR(.brk) - LOAD_OFFSET) {
+	.brk : AT(ADDR(.brk) - LOAD_OFFSET) {
 		__brk_base = .;
 		. += 64 * 1024;		/* 64k alignment slop space */
 		*(.bss..brk)		/* areas brk users have reserved */
-- 
2.35.3

Re: [PATCH v2 3/3] x86: fix .brk attribute in linker script

[Josh Poimboeuf]

On Thu, Jun 23, 2022 at 11:46:08AM +0200, Juergen Gross wrote:
> Commit e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
> added the "NOLOAD" attribute to the .brk section as a "failsafe"
> measure.
> 
> Unfortunately this leads to the linker no longer covering the .brk
> section in a program header, resulting in the kernel loader not knowing
> that the memory for the .brk section must be reserved.
> 
> This has led to crashes when loading the kernel as PV dom0 under Xen,
> but other scenarios could be hit by the same problem (e.g. in case an
> uncompressed kernel is used and the initrd is placed directly behind
> it).
> 
> So drop the "NOLOAD" attribute. This has been verified to correctly
> cover the .brk section by a program header of the resulting ELF file.
> 
> Fixes: e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Josh Poimboeuf <jpoimboe@kernel.org>

-- 
Josh

Re: [PATCH v2 0/3] x86: fix brk area initialization

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 872 bytes --]

On 23.06.22 11:46, Juergen Gross wrote:
> The brk area needs to be zeroed initially, like the .bss section.
> At the same time its memory should be covered by the ELF program
> headers.
> 
> Juergen Gross (3):
>    x86/xen: use clear_bss() for Xen PV guests
>    x86: fix setup of brk area
>    x86: fix .brk attribute in linker script
> 
>   arch/x86/include/asm/setup.h  |  3 +++
>   arch/x86/kernel/head64.c      |  4 +++-
>   arch/x86/kernel/vmlinux.lds.S |  2 +-
>   arch/x86/xen/enlighten_pv.c   |  8 ++++++--
>   arch/x86/xen/xen-head.S       | 10 +---------
>   5 files changed, 14 insertions(+), 13 deletions(-)
> 

Could I please have some feedback? This series is fixing a major
regression regarding running as Xen PV guest (depending on kernel
configuration system will crash very early).

#regzbot ^introduced e32683c6f7d2


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 0/3] x86: fix brk area initialization

[Boris Ostrovsky]

On 6/29/22 10:10 AM, Juergen Gross wrote:
> On 23.06.22 11:46, Juergen Gross wrote:
>> The brk area needs to be zeroed initially, like the .bss section.
>> At the same time its memory should be covered by the ELF program
>> headers.
>>
>> Juergen Gross (3):
>>    x86/xen: use clear_bss() for Xen PV guests
>>    x86: fix setup of brk area
>>    x86: fix .brk attribute in linker script
>>
>>   arch/x86/include/asm/setup.h  |  3 +++
>>   arch/x86/kernel/head64.c      |  4 +++-
>>   arch/x86/kernel/vmlinux.lds.S |  2 +-
>>   arch/x86/xen/enlighten_pv.c   |  8 ++++++--
>>   arch/x86/xen/xen-head.S       | 10 +---------
>>   5 files changed, 14 insertions(+), 13 deletions(-)
>>
>
> Could I please have some feedback? This series is fixing a major
> regression regarding running as Xen PV guest (depending on kernel
> configuration system will crash very early).
>
> #regzbot ^introduced e32683c6f7d2
>


I don't think you need this for Xen bits as Jan had already reviewed it but in case you do


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>