[PATCH v3 0/5] Adjust the padding size for KASLR

[PATCH v3 0/5] Adjust the padding size for KASLR

[Masayoshi Mizuma]

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

The system sometimes crashes while memory hot-adding on KASLR
enabled system. The crash happens because the regions pointed by
kaslr_regions[].base are overwritten by the hot-added memory.

It happens because of the padding size for kaslr_regions[].base isn't
enough for the system whose physical memory layout has huge space for
memory hotplug. kaslr_regions[].base points "actual installed
memory size + padding" or higher address. So, if the "actual + padding"
is lower address than the maximum memory address, which means the memory
address reachable by memory hot-add, kaslr_regions[].base is destroyed by
the overwritten.

  address
    ^
    |------- maximum memory address (Hotplug)
    |                                    ^
    |------- kaslr_regions[0].base       | Hotadd-able region
    |     ^                              |
    |     | padding                      |
    |     V                              V
    |------- actual memory address (Installed on boot)
    |

Fix it by getting the maximum memory address from SRAT and store
the value in boot_param, then set the padding size while KASLR
initializing if the default padding size isn't enough.

Masayoshi Mizuma (5):
  x86/boot: Wrap up the SRAT traversing code into subtable_parse()
  x86/boot: Add max_addr field in struct boot_params
  x86/boot: Get the max address from SRAT
  x86/mm/KASLR: Cleanup calculation for direct mapping size
  x86/mm/KASLR: Adjust the padding size for the direct mapping.

 Documentation/x86/zero-page.rst       |  4 ++
 arch/x86/boot/compressed/acpi.c       | 33 +++++++++---
 arch/x86/include/uapi/asm/bootparam.h |  2 +-
 arch/x86/mm/kaslr.c                   | 77 +++++++++++++++++++++------
 4 files changed, 93 insertions(+), 23 deletions(-)

-- 
2.18.1

[PATCH v3 1/5] x86/boot: Wrap up the SRAT traversing code into subtable_parse()

[Masayoshi Mizuma]

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Wrap up the SRAT traversing code into subtable_parse().

Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 arch/x86/boot/compressed/acpi.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 149795c36..908a1bfab 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -362,6 +362,19 @@ static unsigned long get_acpi_srat_table(void)
 	return 0;
 }
 
+static void subtable_parse(struct acpi_subtable_header *sub_table, int *num)
+{
+	struct acpi_srat_mem_affinity *ma;
+
+	ma = (struct acpi_srat_mem_affinity *)sub_table;
+
+	if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) {
+		immovable_mem[*num].start = ma->base_address;
+		immovable_mem[*num].size = ma->length;
+		(*num)++;
+	}
+}
+
 /**
  * count_immovable_mem_regions - Parse SRAT and cache the immovable
  * memory regions into the immovable_mem array.
@@ -395,14 +408,8 @@ int count_immovable_mem_regions(void)
 	while (table + sizeof(struct acpi_subtable_header) < table_end) {
 		sub_table = (struct acpi_subtable_header *)table;
 		if (sub_table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
-			struct acpi_srat_mem_affinity *ma;
 
-			ma = (struct acpi_srat_mem_affinity *)sub_table;
-			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) {
-				immovable_mem[num].start = ma->base_address;
-				immovable_mem[num].size = ma->length;
-				num++;
-			}
+			subtable_parse(sub_table, &num);
 
 			if (num >= MAX_NUMNODES*2) {
 				debug_putstr("Too many immovable memory regions, aborting.\n");
-- 
2.18.1

[PATCH v3 2/5] x86/boot: Add max_addr field in struct boot_params

[Masayoshi Mizuma]

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Add max_addr field in struct boot_params. max_addr shows the
maximum memory address to be reachable by memory hot-add.
max_addr is set by parsing ACPI SRAT.

Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 Documentation/x86/zero-page.rst       | 4 ++++
 arch/x86/include/uapi/asm/bootparam.h | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/x86/zero-page.rst b/Documentation/x86/zero-page.rst
index f088f5881..cc3938d68 100644
--- a/Documentation/x86/zero-page.rst
+++ b/Documentation/x86/zero-page.rst
@@ -19,6 +19,7 @@ Offset/Size	Proto	Name			Meaning
 058/008		ALL	tboot_addr      	Physical address of tboot shared page
 060/010		ALL	ist_info		Intel SpeedStep (IST) BIOS support information
 						(struct ist_info)
+078/010		ALL	max_addr		The possible maximum physical memory address [1]_
 080/010		ALL	hd0_info		hd0 disk parameter, OBSOLETE!!
 090/010		ALL	hd1_info		hd1 disk parameter, OBSOLETE!!
 0A0/010		ALL	sys_desc_table		System description table (struct sys_desc_table),
@@ -43,3 +44,6 @@ Offset/Size	Proto	Name			Meaning
 						(array of struct e820_entry)
 D00/1EC		ALL	eddbuf			EDD data (array of struct edd_info)
 ===========	=====	=======================	=================================================
+
+.. [1] max_addr shows the maximum memory address to be reachable by memory
+       hot-add. max_addr is set by parsing ACPI SRAT.
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c895df548..6efad338b 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -158,7 +158,7 @@ struct boot_params {
 	__u64  tboot_addr;				/* 0x058 */
 	struct ist_info ist_info;			/* 0x060 */
 	__u64 acpi_rsdp_addr;				/* 0x070 */
-	__u8  _pad3[8];					/* 0x078 */
+	__u64 max_addr;					/* 0x078 */
 	__u8  hd0_info[16];	/* obsolete! */		/* 0x080 */
 	__u8  hd1_info[16];	/* obsolete! */		/* 0x090 */
 	struct sys_desc_table sys_desc_table; /* obsolete! */	/* 0x0a0 */
-- 
2.18.1

[PATCH v3 3/5] x86/boot: Get the max address from SRAT

[Masayoshi Mizuma]

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Get the max address from SRAT and write it into boot_params->max_addr.

Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 arch/x86/boot/compressed/acpi.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 908a1bfab..ba2bc5ab9 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -362,16 +362,24 @@ static unsigned long get_acpi_srat_table(void)
 	return 0;
 }
 
-static void subtable_parse(struct acpi_subtable_header *sub_table, int *num)
+static void subtable_parse(struct acpi_subtable_header *sub_table, int *num,
+		unsigned long *max_addr)
 {
 	struct acpi_srat_mem_affinity *ma;
+	unsigned long addr;
 
 	ma = (struct acpi_srat_mem_affinity *)sub_table;
 
-	if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) {
-		immovable_mem[*num].start = ma->base_address;
-		immovable_mem[*num].size = ma->length;
-		(*num)++;
+	if (ma->length) {
+		if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
+			addr = ma->base_address + ma->length;
+			if (addr > *max_addr)
+				*max_addr = addr;
+		} else {
+			immovable_mem[*num].start = ma->base_address;
+			immovable_mem[*num].size = ma->length;
+			(*num)++;
+		}
 	}
 }
 
@@ -391,6 +399,7 @@ int count_immovable_mem_regions(void)
 	struct acpi_subtable_header *sub_table;
 	struct acpi_table_header *table_header;
 	char arg[MAX_ACPI_ARG_LENGTH];
+	unsigned long max_addr = 0;
 	int num = 0;
 
 	if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
@@ -409,7 +418,7 @@ int count_immovable_mem_regions(void)
 		sub_table = (struct acpi_subtable_header *)table;
 		if (sub_table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
 
-			subtable_parse(sub_table, &num);
+			subtable_parse(sub_table, &num, &max_addr);
 
 			if (num >= MAX_NUMNODES*2) {
 				debug_putstr("Too many immovable memory regions, aborting.\n");
@@ -418,6 +427,9 @@ int count_immovable_mem_regions(void)
 		}
 		table += sub_table->length;
 	}
+
+	boot_params->max_addr = max_addr;
+
 	return num;
 }
 #endif /* CONFIG_RANDOMIZE_BASE && CONFIG_MEMORY_HOTREMOVE */
-- 
2.18.1

[PATCH v3 4/5] x86/mm/KASLR: Cleanup calculation for direct mapping size

[Masayoshi Mizuma]

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Cleanup calculation for direct mapping size.

Signed-off-by: Baoquan He <bhe@redhat.com>
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 arch/x86/mm/kaslr.c | 50 +++++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
index dc6182eec..8e5f3642e 100644
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -70,15 +70,45 @@ static inline bool kaslr_memory_enabled(void)
 	return kaslr_enabled() && !IS_ENABLED(CONFIG_KASAN);
 }
 
+/*
+ * Even though a huge virtual address space is reserved for the direct
+ * mapping of physical memory, e.g in 4-level paging mode, it's 64TB,
+ * rare system can own enough physical memory to use it up, most are
+ * even less than 1TB. So with KASLR enabled, we adapt the size of
+ * direct mapping area to the size of actual physical memory plus the
+ * configured padding CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING.
+ * The left part will be taken out to join memory randomization.
+ */
+static inline unsigned long calc_direct_mapping_size(void)
+{
+	unsigned long size_tb, memory_tb;
+
+	/*
+	 * Update Physical memory mapping to available and
+	 * add padding if needed (especially for memory hotplug support).
+	 */
+	memory_tb = DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
+		CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
+
+	size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
+
+	/*
+	 * Adapt physical memory region size based on available memory
+	 */
+	if (memory_tb < size_tb)
+		size_tb = memory_tb;
+
+	return size_tb;
+}
+
 /* Initialize base and padding for each memory region randomized with KASLR */
 void __init kernel_randomize_memory(void)
 {
-	size_t i;
-	unsigned long vaddr_start, vaddr;
-	unsigned long rand, memory_tb;
-	struct rnd_state rand_state;
+	unsigned long vaddr_start, vaddr, rand;
 	unsigned long remain_entropy;
 	unsigned long vmemmap_size;
+	struct rnd_state rand_state;
+	size_t i;
 
 	vaddr_start = pgtable_l5_enabled() ? __PAGE_OFFSET_BASE_L5 : __PAGE_OFFSET_BASE_L4;
 	vaddr = vaddr_start;
@@ -95,20 +125,10 @@ void __init kernel_randomize_memory(void)
 	if (!kaslr_memory_enabled())
 		return;
 
-	kaslr_regions[0].size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
+	kaslr_regions[0].size_tb = calc_direct_mapping_size();
 	kaslr_regions[1].size_tb = VMALLOC_SIZE_TB;
 
-	/*
-	 * Update Physical memory mapping to available and
-	 * add padding if needed (especially for memory hotplug support).
-	 */
 	BUG_ON(kaslr_regions[0].base != &page_offset_base);
-	memory_tb = DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
-		CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
-
-	/* Adapt phyiscal memory region size based on available memory */
-	if (memory_tb < kaslr_regions[0].size_tb)
-		kaslr_regions[0].size_tb = memory_tb;
 
 	/*
 	 * Calculate the vmemmap region size in TBs, aligned to a TB
-- 
2.18.1

[PATCH v3 5/5] x86/mm/KASLR: Adjust the padding size for the direct mapping.

[Masayoshi Mizuma]

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

The system sometimes crashes while memory hot-adding on KASLR
enabled system. The crash happens because the regions pointed by
kaslr_regions[].base are overwritten by the hot-added memory.

It happens because of the padding size for kaslr_regions[].base isn't
enough for the system whose physical memory layout has huge space for
memory hotplug. kaslr_regions[].base points "actual installed
memory size + padding" or higher address. So, if the "actual + padding"
is lower address than the maximum memory address, which means the memory
address reachable by memory hot-add, kaslr_regions[].base is destroyed by
the overwritten.

  address
    ^
    |------- maximum memory address (Hotplug)
    |                                    ^
    |------- kaslr_regions[0].base       | Hotadd-able region
    |     ^                              |
    |     | padding                      |
    |     V                              V
    |------- actual memory address (Installed on boot)
    |

Fix it by getting the maximum memory address from SRAT and store
the value in boot_param, then set the padding size while KASLR
initializing if the default padding size isn't enough.

Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 arch/x86/mm/kaslr.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
index 8e5f3642e..a78844c57 100644
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -70,6 +70,34 @@ static inline bool kaslr_memory_enabled(void)
 	return kaslr_enabled() && !IS_ENABLED(CONFIG_KASAN);
 }
 
+static inline unsigned long phys_memmap_size(void)
+{
+	unsigned long padding = CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
+#ifdef CONFIG_MEMORY_HOTPLUG
+	unsigned long actual, maximum, base;
+
+	if (!boot_params.max_addr)
+		goto out;
+
+	/*
+	 * The padding size should set to get for kaslr_regions[].base
+	 * bigger address than the maximum memory address the system can
+	 * have. kaslr_regions[].base points "actual size + padding" or
+	 * higher address. If "actual size + padding" points the lower
+	 * address than the maximum memory size, fix the padding size.
+	 */
+	actual = roundup(PFN_PHYS(max_pfn), 1UL << TB_SHIFT);
+	maximum = roundup(boot_params.max_addr, 1UL << TB_SHIFT);
+	base = actual + (padding << TB_SHIFT);
+
+	if (maximum > base)
+		padding = (maximum - actual) >> TB_SHIFT;
+out:
+#endif
+	return DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
+			padding;
+}
+
 /*
  * Even though a huge virtual address space is reserved for the direct
  * mapping of physical memory, e.g in 4-level paging mode, it's 64TB,
@@ -87,8 +115,7 @@ static inline unsigned long calc_direct_mapping_size(void)
 	 * Update Physical memory mapping to available and
 	 * add padding if needed (especially for memory hotplug support).
 	 */
-	memory_tb = DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
-		CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
+	memory_tb = phys_memmap_size();
 
 	size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
 
-- 
2.18.1

Re: [PATCH v3 1/5] x86/boot: Wrap up the SRAT traversing code into subtable_parse()

[Baoquan He]

On 08/30/19 at 05:47pm, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> Wrap up the SRAT traversing code into subtable_parse().
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpi.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 149795c36..908a1bfab 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -362,6 +362,19 @@ static unsigned long get_acpi_srat_table(void)
>  	return 0;
>  }

Reviewed-by: Baoquan He <bhe@redhat.com>

Thanks
Baoquan
>  
> +static void subtable_parse(struct acpi_subtable_header *sub_table, int *num)
> +{
> +	struct acpi_srat_mem_affinity *ma;
> +
> +	ma = (struct acpi_srat_mem_affinity *)sub_table;
> +
> +	if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) {
> +		immovable_mem[*num].start = ma->base_address;
> +		immovable_mem[*num].size = ma->length;
> +		(*num)++;
> +	}
> +}
> +
>  /**
>   * count_immovable_mem_regions - Parse SRAT and cache the immovable
>   * memory regions into the immovable_mem array.
> @@ -395,14 +408,8 @@ int count_immovable_mem_regions(void)
>  	while (table + sizeof(struct acpi_subtable_header) < table_end) {
>  		sub_table = (struct acpi_subtable_header *)table;
>  		if (sub_table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
> -			struct acpi_srat_mem_affinity *ma;
>  
> -			ma = (struct acpi_srat_mem_affinity *)sub_table;
> -			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) {
> -				immovable_mem[num].start = ma->base_address;
> -				immovable_mem[num].size = ma->length;
> -				num++;
> -			}
> +			subtable_parse(sub_table, &num);
>  
>  			if (num >= MAX_NUMNODES*2) {
>  				debug_putstr("Too many immovable memory regions, aborting.\n");
> -- 
> 2.18.1
> 

Re: [PATCH v3 2/5] x86/boot: Add max_addr field in struct boot_params

[Baoquan He]

On 08/30/19 at 05:47pm, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> Add max_addr field in struct boot_params. max_addr shows the
> maximum memory address to be reachable by memory hot-add.
> max_addr is set by parsing ACPI SRAT.
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> ---
>  Documentation/x86/zero-page.rst       | 4 ++++
>  arch/x86/include/uapi/asm/bootparam.h | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Baoquan He <bhe@redhat.com>

Thanks
Baoquan

> 
> diff --git a/Documentation/x86/zero-page.rst b/Documentation/x86/zero-page.rst
> index f088f5881..cc3938d68 100644
> --- a/Documentation/x86/zero-page.rst
> +++ b/Documentation/x86/zero-page.rst
> @@ -19,6 +19,7 @@ Offset/Size	Proto	Name			Meaning
>  058/008		ALL	tboot_addr      	Physical address of tboot shared page
>  060/010		ALL	ist_info		Intel SpeedStep (IST) BIOS support information
>  						(struct ist_info)
> +078/010		ALL	max_addr		The possible maximum physical memory address [1]_
>  080/010		ALL	hd0_info		hd0 disk parameter, OBSOLETE!!
>  090/010		ALL	hd1_info		hd1 disk parameter, OBSOLETE!!
>  0A0/010		ALL	sys_desc_table		System description table (struct sys_desc_table),
> @@ -43,3 +44,6 @@ Offset/Size	Proto	Name			Meaning
>  						(array of struct e820_entry)
>  D00/1EC		ALL	eddbuf			EDD data (array of struct edd_info)
>  ===========	=====	=======================	=================================================
> +
> +.. [1] max_addr shows the maximum memory address to be reachable by memory
> +       hot-add. max_addr is set by parsing ACPI SRAT.
> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> index c895df548..6efad338b 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -158,7 +158,7 @@ struct boot_params {
>  	__u64  tboot_addr;				/* 0x058 */
>  	struct ist_info ist_info;			/* 0x060 */
>  	__u64 acpi_rsdp_addr;				/* 0x070 */
> -	__u8  _pad3[8];					/* 0x078 */
> +	__u64 max_addr;					/* 0x078 */
>  	__u8  hd0_info[16];	/* obsolete! */		/* 0x080 */
>  	__u8  hd1_info[16];	/* obsolete! */		/* 0x090 */
>  	struct sys_desc_table sys_desc_table; /* obsolete! */	/* 0x0a0 */
> -- 
> 2.18.1
> 

Re: [PATCH v3 3/5] x86/boot: Get the max address from SRAT

[Baoquan He]

On 08/30/19 at 05:47pm, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> Get the max address from SRAT and write it into boot_params->max_addr.
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpi.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 908a1bfab..ba2bc5ab9 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -362,16 +362,24 @@ static unsigned long get_acpi_srat_table(void)
>  	return 0;
>  }
>  
> -static void subtable_parse(struct acpi_subtable_header *sub_table, int *num)
> +static void subtable_parse(struct acpi_subtable_header *sub_table, int *num,
> +		unsigned long *max_addr)
>  {
>  	struct acpi_srat_mem_affinity *ma;
> +	unsigned long addr;
>  
>  	ma = (struct acpi_srat_mem_affinity *)sub_table;
>  
> -	if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) {
> -		immovable_mem[*num].start = ma->base_address;
> -		immovable_mem[*num].size = ma->length;
> -		(*num)++;
> +	if (ma->length) {
> +		if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> +			addr = ma->base_address + ma->length;
> +			if (addr > *max_addr)
> +				*max_addr = addr;

Can we return max_addr or only pass out the max_addr, then let the
max_addr compared and got outside of subtable_parse()? This can keep
subtable_parse() really only doing parsing work.

Personal opinion, see what maintainers and other reviewers will say.

Thanks
Baoquan

> +		} else {
> +			immovable_mem[*num].start = ma->base_address;
> +			immovable_mem[*num].size = ma->length;
> +			(*num)++;
> +		}
>  	}
>  }
>  
> @@ -391,6 +399,7 @@ int count_immovable_mem_regions(void)
>  	struct acpi_subtable_header *sub_table;
>  	struct acpi_table_header *table_header;
>  	char arg[MAX_ACPI_ARG_LENGTH];
> +	unsigned long max_addr = 0;
>  	int num = 0;
>  
>  	if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
> @@ -409,7 +418,7 @@ int count_immovable_mem_regions(void)
>  		sub_table = (struct acpi_subtable_header *)table;
>  		if (sub_table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
>  
> -			subtable_parse(sub_table, &num);
> +			subtable_parse(sub_table, &num, &max_addr);
>  
>  			if (num >= MAX_NUMNODES*2) {
>  				debug_putstr("Too many immovable memory regions, aborting.\n");
> @@ -418,6 +427,9 @@ int count_immovable_mem_regions(void)
>  		}
>  		table += sub_table->length;
>  	}
> +
> +	boot_params->max_addr = max_addr;
> +
>  	return num;
>  }
>  #endif /* CONFIG_RANDOMIZE_BASE && CONFIG_MEMORY_HOTREMOVE */
> -- 
> 2.18.1
> 

Re: [PATCH v3 4/5] x86/mm/KASLR: Cleanup calculation for direct mapping size

[Baoquan He]

On 08/30/19 at 05:47pm, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> Cleanup calculation for direct mapping size.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> ---
>  arch/x86/mm/kaslr.c | 50 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> index dc6182eec..8e5f3642e 100644
> --- a/arch/x86/mm/kaslr.c
> +++ b/arch/x86/mm/kaslr.c
> @@ -70,15 +70,45 @@ static inline bool kaslr_memory_enabled(void)
>  	return kaslr_enabled() && !IS_ENABLED(CONFIG_KASAN);
>  }
>  
> +/*
> + * Even though a huge virtual address space is reserved for the direct
> + * mapping of physical memory, e.g in 4-level paging mode, it's 64TB,
> + * rare system can own enough physical memory to use it up, most are
> + * even less than 1TB. So with KASLR enabled, we adapt the size of
> + * direct mapping area to the size of actual physical memory plus the
> + * configured padding CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING.
> + * The left part will be taken out to join memory randomization.
> + */
> +static inline unsigned long calc_direct_mapping_size(void)

I think patch 4 and 5 can be merged, just keep one
calc_direct_mapping_size() to do the mapping size calculation for the
direct mapping section, it's not that complicated. Adding
phys_memmap_size() makes it a little redundent, in my opinion.

Thanks
Baoquan

> +{
> +	unsigned long size_tb, memory_tb;
> +
> +	/*
> +	 * Update Physical memory mapping to available and
> +	 * add padding if needed (especially for memory hotplug support).
> +	 */
> +	memory_tb = DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
> +		CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
> +
> +	size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
> +
> +	/*
> +	 * Adapt physical memory region size based on available memory
> +	 */
> +	if (memory_tb < size_tb)
> +		size_tb = memory_tb;
> +
> +	return size_tb;
> +}
> +
>  /* Initialize base and padding for each memory region randomized with KASLR */
>  void __init kernel_randomize_memory(void)
>  {
> -	size_t i;
> -	unsigned long vaddr_start, vaddr;
> -	unsigned long rand, memory_tb;
> -	struct rnd_state rand_state;
> +	unsigned long vaddr_start, vaddr, rand;
>  	unsigned long remain_entropy;
>  	unsigned long vmemmap_size;
> +	struct rnd_state rand_state;
> +	size_t i;
>  
>  	vaddr_start = pgtable_l5_enabled() ? __PAGE_OFFSET_BASE_L5 : __PAGE_OFFSET_BASE_L4;
>  	vaddr = vaddr_start;
> @@ -95,20 +125,10 @@ void __init kernel_randomize_memory(void)
>  	if (!kaslr_memory_enabled())
>  		return;
>  
> -	kaslr_regions[0].size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
> +	kaslr_regions[0].size_tb = calc_direct_mapping_size();
>  	kaslr_regions[1].size_tb = VMALLOC_SIZE_TB;
>  
> -	/*
> -	 * Update Physical memory mapping to available and
> -	 * add padding if needed (especially for memory hotplug support).
> -	 */
>  	BUG_ON(kaslr_regions[0].base != &page_offset_base);
> -	memory_tb = DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
> -		CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
> -
> -	/* Adapt phyiscal memory region size based on available memory */
> -	if (memory_tb < kaslr_regions[0].size_tb)
> -		kaslr_regions[0].size_tb = memory_tb;
>  
>  	/*
>  	 * Calculate the vmemmap region size in TBs, aligned to a TB
> -- 
> 2.18.1
> 

Re: [PATCH v3 0/5] Adjust the padding size for KASLR

[Baoquan He]

Hi Masa,

On 08/30/19 at 05:47pm, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Any plan about this patchset?

Thanks
Baoquan

> 
> The system sometimes crashes while memory hot-adding on KASLR
> enabled system. The crash happens because the regions pointed by
> kaslr_regions[].base are overwritten by the hot-added memory.
> 
> It happens because of the padding size for kaslr_regions[].base isn't
> enough for the system whose physical memory layout has huge space for
> memory hotplug. kaslr_regions[].base points "actual installed
> memory size + padding" or higher address. So, if the "actual + padding"
> is lower address than the maximum memory address, which means the memory
> address reachable by memory hot-add, kaslr_regions[].base is destroyed by
> the overwritten.
> 
>   address
>     ^
>     |------- maximum memory address (Hotplug)
>     |                                    ^
>     |------- kaslr_regions[0].base       | Hotadd-able region
>     |     ^                              |
>     |     | padding                      |
>     |     V                              V
>     |------- actual memory address (Installed on boot)
>     |
> 
> Fix it by getting the maximum memory address from SRAT and store
> the value in boot_param, then set the padding size while KASLR
> initializing if the default padding size isn't enough.
> 
> Masayoshi Mizuma (5):
>   x86/boot: Wrap up the SRAT traversing code into subtable_parse()
>   x86/boot: Add max_addr field in struct boot_params
>   x86/boot: Get the max address from SRAT
>   x86/mm/KASLR: Cleanup calculation for direct mapping size
>   x86/mm/KASLR: Adjust the padding size for the direct mapping.
> 
>  Documentation/x86/zero-page.rst       |  4 ++
>  arch/x86/boot/compressed/acpi.c       | 33 +++++++++---
>  arch/x86/include/uapi/asm/bootparam.h |  2 +-
>  arch/x86/mm/kaslr.c                   | 77 +++++++++++++++++++++------
>  4 files changed, 93 insertions(+), 23 deletions(-)
> 
> -- 
> 2.18.1
> 

Re: [PATCH v3 3/5] x86/boot: Get the max address from SRAT

[Masayoshi Mizuma]

On Thu, Sep 05, 2019 at 09:51:34PM +0800, Baoquan He wrote:
> On 08/30/19 at 05:47pm, Masayoshi Mizuma wrote:
> > From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> > 
> > Get the max address from SRAT and write it into boot_params->max_addr.
> > 
> > Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> > ---
> >  arch/x86/boot/compressed/acpi.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> > index 908a1bfab..ba2bc5ab9 100644
> > --- a/arch/x86/boot/compressed/acpi.c
> > +++ b/arch/x86/boot/compressed/acpi.c
> > @@ -362,16 +362,24 @@ static unsigned long get_acpi_srat_table(void)
> >  	return 0;
> >  }
> >  
> > -static void subtable_parse(struct acpi_subtable_header *sub_table, int *num)
> > +static void subtable_parse(struct acpi_subtable_header *sub_table, int *num,
> > +		unsigned long *max_addr)
> >  {
> >  	struct acpi_srat_mem_affinity *ma;
> > +	unsigned long addr;
> >  
> >  	ma = (struct acpi_srat_mem_affinity *)sub_table;
> >  
> > -	if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) {
> > -		immovable_mem[*num].start = ma->base_address;
> > -		immovable_mem[*num].size = ma->length;
> > -		(*num)++;
> > +	if (ma->length) {
> > +		if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> > +			addr = ma->base_address + ma->length;
> > +			if (addr > *max_addr)
> > +				*max_addr = addr;
> 
> Can we return max_addr or only pass out the max_addr, then let the
> max_addr compared and got outside of subtable_parse()? This can keep
> subtable_parse() really only doing parsing work.

Sounds great! I'll change subtable_parse() to return max_addr.

Thanks,
Masa

> 
> Personal opinion, see what maintainers and other reviewers will say.
> 
> Thanks
> Baoquan
> 
> > +		} else {
> > +			immovable_mem[*num].start = ma->base_address;
> > +			immovable_mem[*num].size = ma->length;
> > +			(*num)++;
> > +		}
> >  	}
> >  }
> >  
> > @@ -391,6 +399,7 @@ int count_immovable_mem_regions(void)
> >  	struct acpi_subtable_header *sub_table;
> >  	struct acpi_table_header *table_header;
> >  	char arg[MAX_ACPI_ARG_LENGTH];
> > +	unsigned long max_addr = 0;
> >  	int num = 0;
> >  
> >  	if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
> > @@ -409,7 +418,7 @@ int count_immovable_mem_regions(void)
> >  		sub_table = (struct acpi_subtable_header *)table;
> >  		if (sub_table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
> >  
> > -			subtable_parse(sub_table, &num);
> > +			subtable_parse(sub_table, &num, &max_addr);
> >  
> >  			if (num >= MAX_NUMNODES*2) {
> >  				debug_putstr("Too many immovable memory regions, aborting.\n");
> > @@ -418,6 +427,9 @@ int count_immovable_mem_regions(void)
> >  		}
> >  		table += sub_table->length;
> >  	}
> > +
> > +	boot_params->max_addr = max_addr;
> > +
> >  	return num;
> >  }
> >  #endif /* CONFIG_RANDOMIZE_BASE && CONFIG_MEMORY_HOTREMOVE */
> > -- 
> > 2.18.1
> > 

Re: [PATCH v3 4/5] x86/mm/KASLR: Cleanup calculation for direct mapping size

[Masayoshi Mizuma]

On Thu, Sep 05, 2019 at 09:54:51PM +0800, Baoquan He wrote:
> On 08/30/19 at 05:47pm, Masayoshi Mizuma wrote:
> > From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> > 
> > Cleanup calculation for direct mapping size.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> > ---
> >  arch/x86/mm/kaslr.c | 50 +++++++++++++++++++++++++++++++--------------
> >  1 file changed, 35 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> > index dc6182eec..8e5f3642e 100644
> > --- a/arch/x86/mm/kaslr.c
> > +++ b/arch/x86/mm/kaslr.c
> > @@ -70,15 +70,45 @@ static inline bool kaslr_memory_enabled(void)
> >  	return kaslr_enabled() && !IS_ENABLED(CONFIG_KASAN);
> >  }
> >  
> > +/*
> > + * Even though a huge virtual address space is reserved for the direct
> > + * mapping of physical memory, e.g in 4-level paging mode, it's 64TB,
> > + * rare system can own enough physical memory to use it up, most are
> > + * even less than 1TB. So with KASLR enabled, we adapt the size of
> > + * direct mapping area to the size of actual physical memory plus the
> > + * configured padding CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING.
> > + * The left part will be taken out to join memory randomization.
> > + */
> > +static inline unsigned long calc_direct_mapping_size(void)
> 
> I think patch 4 and 5 can be merged, just keep one
> calc_direct_mapping_size() to do the mapping size calculation for the
> direct mapping section, it's not that complicated. Adding
> phys_memmap_size() makes it a little redundent, in my opinion.

Thanks, I'll merge patch 4 and 5.

- Masa

> 
> Thanks
> Baoquan
> 
> > +{
> > +	unsigned long size_tb, memory_tb;
> > +
> > +	/*
> > +	 * Update Physical memory mapping to available and
> > +	 * add padding if needed (especially for memory hotplug support).
> > +	 */
> > +	memory_tb = DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
> > +		CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
> > +
> > +	size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
> > +
> > +	/*
> > +	 * Adapt physical memory region size based on available memory
> > +	 */
> > +	if (memory_tb < size_tb)
> > +		size_tb = memory_tb;
> > +
> > +	return size_tb;
> > +}
> > +
> >  /* Initialize base and padding for each memory region randomized with KASLR */
> >  void __init kernel_randomize_memory(void)
> >  {
> > -	size_t i;
> > -	unsigned long vaddr_start, vaddr;
> > -	unsigned long rand, memory_tb;
> > -	struct rnd_state rand_state;
> > +	unsigned long vaddr_start, vaddr, rand;
> >  	unsigned long remain_entropy;
> >  	unsigned long vmemmap_size;
> > +	struct rnd_state rand_state;
> > +	size_t i;
> >  
> >  	vaddr_start = pgtable_l5_enabled() ? __PAGE_OFFSET_BASE_L5 : __PAGE_OFFSET_BASE_L4;
> >  	vaddr = vaddr_start;
> > @@ -95,20 +125,10 @@ void __init kernel_randomize_memory(void)
> >  	if (!kaslr_memory_enabled())
> >  		return;
> >  
> > -	kaslr_regions[0].size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
> > +	kaslr_regions[0].size_tb = calc_direct_mapping_size();
> >  	kaslr_regions[1].size_tb = VMALLOC_SIZE_TB;
> >  
> > -	/*
> > -	 * Update Physical memory mapping to available and
> > -	 * add padding if needed (especially for memory hotplug support).
> > -	 */
> >  	BUG_ON(kaslr_regions[0].base != &page_offset_base);
> > -	memory_tb = DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
> > -		CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
> > -
> > -	/* Adapt phyiscal memory region size based on available memory */
> > -	if (memory_tb < kaslr_regions[0].size_tb)
> > -		kaslr_regions[0].size_tb = memory_tb;
> >  
> >  	/*
> >  	 * Calculate the vmemmap region size in TBs, aligned to a TB
> > -- 
> > 2.18.1
> > 

Re: [PATCH v3 0/5] Adjust the padding size for KASLR

[Masayoshi Mizuma]

Hi Baoquan,

On Tue, Oct 29, 2019 at 10:59:20AM +0800, Baoquan He wrote:
> Hi Masa,
> 
> On 08/30/19 at 05:47pm, Masayoshi Mizuma wrote:
> > From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> Any plan about this patchset?

Thank you for pinging me and so sorry for the delay.
I'll post the v4 in this week.

Thanks,
Masa

> 
> Thanks
> Baoquan
> 
> > 
> > The system sometimes crashes while memory hot-adding on KASLR
> > enabled system. The crash happens because the regions pointed by
> > kaslr_regions[].base are overwritten by the hot-added memory.
> > 
> > It happens because of the padding size for kaslr_regions[].base isn't
> > enough for the system whose physical memory layout has huge space for
> > memory hotplug. kaslr_regions[].base points "actual installed
> > memory size + padding" or higher address. So, if the "actual + padding"
> > is lower address than the maximum memory address, which means the memory
> > address reachable by memory hot-add, kaslr_regions[].base is destroyed by
> > the overwritten.
> > 
> >   address
> >     ^
> >     |------- maximum memory address (Hotplug)
> >     |                                    ^
> >     |------- kaslr_regions[0].base       | Hotadd-able region
> >     |     ^                              |
> >     |     | padding                      |
> >     |     V                              V
> >     |------- actual memory address (Installed on boot)
> >     |
> > 
> > Fix it by getting the maximum memory address from SRAT and store
> > the value in boot_param, then set the padding size while KASLR
> > initializing if the default padding size isn't enough.
> > 
> > Masayoshi Mizuma (5):
> >   x86/boot: Wrap up the SRAT traversing code into subtable_parse()
> >   x86/boot: Add max_addr field in struct boot_params
> >   x86/boot: Get the max address from SRAT
> >   x86/mm/KASLR: Cleanup calculation for direct mapping size
> >   x86/mm/KASLR: Adjust the padding size for the direct mapping.
> > 
> >  Documentation/x86/zero-page.rst       |  4 ++
> >  arch/x86/boot/compressed/acpi.c       | 33 +++++++++---
> >  arch/x86/include/uapi/asm/bootparam.h |  2 +-
> >  arch/x86/mm/kaslr.c                   | 77 +++++++++++++++++++++------
> >  4 files changed, 93 insertions(+), 23 deletions(-)
> > 
> > -- 
> > 2.18.1
> > 
>