Hi Boris, Thank you for your review.
On Fri, Feb 08, 2019 at 07:26:00PM +0100, Borislav Petkov wrote: > On Tue, Feb 05, 2019 at 10:05:16AM -0500, Masayoshi Mizuma wrote: > > From: Masayoshi Mizuma <m.miz...@jp.fujitsu.com> > > Date: Tue, 5 Feb 2019 10:00:59 -0500 > > Subject: [PATCH] x86/mm: Introduce adjustment the padding size for KASLR > > "Adjust the padding size for KASLR" > > > If the physical memory layout has huge space for hotplug, the padding > > used for the physical memory mapping section is not enough. > > So, such system may crash while memory hot-adding on KASLR enabled system. > > Crash why? > > Why is the padding not enough? > > > For example, SRAT has the following layout, the maximum possible memory > > size is 32TB, and the memory is installed as 2TB actually, then the padding > > size should set 30TB (== possible memory size - actual memory size). > > > > SRAT: Node 3 PXM 7 [mem 0x1c0000000000-0x1fffffffffff] hotplug > > What is that supposed to exemplify: that range is 3T not 2 and that > range start is not at 2T but 28T. So I have absolutely no clue what > you're trying to explain here. > > Please go back, take your time and structure your commit message like > this: > > Problem is A. > > It happens because of B. > > Fix it by doing C. > > (Potentially do D). > > For more detailed info, see > Documentation/process/submitting-patches.rst, Section "2) Describe your > changes". Got it, thanks. > > > This patch introduces adjustment the padding size if the default > > Avoid having "This patch" or "This commit" in the commit message. It is > tautologically useless. > > Also, do > > $ git grep 'This patch' Documentation/process > > for more details. Thanks. I see. > > > padding size isn't enough. > > > > Signed-off-by: Masayoshi Mizuma <m.miz...@jp.fujitsu.com> > > --- > > Documentation/x86/zero-page.txt | 1 + > > arch/x86/boot/compressed/acpi.c | 19 +++++++++++++++---- > > arch/x86/include/uapi/asm/bootparam.h | 2 +- > > arch/x86/mm/kaslr.c | 26 +++++++++++++++++++++++++- > > 4 files changed, 42 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/x86/zero-page.txt > > b/Documentation/x86/zero-page.txt > > index 68aed077f..343fe1a90 100644 > > --- a/Documentation/x86/zero-page.txt > > +++ b/Documentation/x86/zero-page.txt > > @@ -15,6 +15,7 @@ Offset 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 possible_mem_addr The possible maximum physical memory > > address. > > Why isn't this called max_phys_addr then? > > Also, please explain what it means at the end of this file. > > > 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), > > diff --git a/arch/x86/boot/compressed/acpi.c > > b/arch/x86/boot/compressed/acpi.c > > index c5a949335..7dd61b943 100644 > > --- a/arch/x86/boot/compressed/acpi.c > > +++ b/arch/x86/boot/compressed/acpi.c > > @@ -288,6 +288,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 long possible_addr, max_possible_addr = 0; > > int num = 0; > > > > if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 && > > @@ -308,10 +309,19 @@ int count_immovable_mem_regions(void) > > 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++; > > + if (ma->length) { > > + if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) { > > + possible_addr = > > + ma->base_address + ma->length; > > + if (possible_addr > max_possible_addr) > > + max_possible_addr = > > + possible_addr; > > + } else { > > + immovable_mem[num].start = > > + ma->base_address; > > + immovable_mem[num].size = ma->length; > > + num++; > > + } > > This piece has some ugly linebreaks which makes it impossible to read. > Perhaps because the variable names you're adding are too long. > > You can carve it out in a separate function, for example. Thanks. I will add a separate function like as: 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->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)++; } } } > > > if (num >= MAX_NUMNODES*2) { > > @@ -320,6 +330,7 @@ int count_immovable_mem_regions(void) > > } > > } > > table += sub_table->length; > > + boot_params->possible_mem_addr = max_possible_addr; > > This assignment is inside the loop and happens potentially a bunch of > times. Why? I will move the assigment out of the loop, thanks. > > > } > > return num; > > } > > diff --git a/arch/x86/include/uapi/asm/bootparam.h > > b/arch/x86/include/uapi/asm/bootparam.h > > index 60733f137..5b64b606e 100644 > > --- a/arch/x86/include/uapi/asm/bootparam.h > > +++ b/arch/x86/include/uapi/asm/bootparam.h > > @@ -156,7 +156,7 @@ struct boot_params { > > __u64 tboot_addr; /* 0x058 */ > > struct ist_info ist_info; /* 0x060 */ > > __u64 acpi_rsdp_addr; /* 0x070 */ > > - __u8 _pad3[8]; /* 0x078 */ > > + __u64 possible_mem_addr; /* 0x078 */ > > __u8 hd0_info[16]; /* obsolete! */ /* 0x080 */ > > __u8 hd1_info[16]; /* obsolete! */ /* 0x090 */ > > struct sys_desc_table sys_desc_table; /* obsolete! */ /* 0x0a0 */ > > diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c > > index 3f452ffed..71fc28570 100644 > > --- a/arch/x86/mm/kaslr.c > > +++ b/arch/x86/mm/kaslr.c > > @@ -70,6 +70,30 @@ static inline bool kaslr_memory_enabled(void) > > return kaslr_enabled() && !IS_ENABLED(CONFIG_KASAN); > > } > > > > +static unsigned int __init kaslr_padding(void) > > +{ > > + unsigned int rand_mem_physical_padding = > > + CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING; > > +#ifdef CONFIG_MEMORY_HOTPLUG > > + unsigned long long max_possible_phys, max_actual_phys, threshold; > > + > > + if (!boot_params.possible_mem_addr) > > + goto out; > > + > > + max_actual_phys = roundup(PFN_PHYS(max_pfn), 1ULL << TB_SHIFT); > > + max_possible_phys = roundup(boot_params.possible_mem_addr, > > + 1ULL << TB_SHIFT); > > + threshold = max_actual_phys + > > + ((unsigned long long)rand_mem_physical_padding << TB_SHIFT); > > + > > + if (max_possible_phys > threshold) > > + rand_mem_physical_padding = > > + (max_possible_phys - max_actual_phys) >> TB_SHIFT; > > +out: > > +#endif > > + return rand_mem_physical_padding; > > Same problem: local variables with unnecessarily long names make the > code hard to read. Pls shorten. > > Also, the types in that function are a total mess. An unsigned int which > you cast to unsigned long long and return an unsigned int again to add > into a sum with unsigned longs. Are you selecting the types randomly? > Why aren't you using plain unsigned longs like the rest of the file does > with memory addresses? Sorry. I will unify the type as unsinged long. > > Also, this function could have a comment above it explaining what all > that threshold and actual and possible address arithmetic is supposed to > do. Got it. Thanks! Masa