[PATCH 0/4] kaslr: extend movable_node to movable_node=nn[KMG]@ss[KMG]

[PATCH 0/4] kaslr: extend movable_node to movable_node=nn[KMG]@ss[KMG]

[Chao Fan]

Here is a problem:
Here is a machine with several NUMA nodes and some of them are hot-pluggable.
It's not good for kernel to be extracted in the memory region of movable node.
But in current code, I print the address choosen by kaslr and found it may be
placed in movable node sometimes. To solve this problem, it's better to limit
the memory region choosen by kaslr to immovable node in kaslr.c. But the memory
infomation about if it's hot-pluggable is stored in ACPI SRAT table, which is
parsed after kernel is extracted. So we can't get the detail memory infomation
before extracting kernel.

So extend the movable_node to movable_node=nn@ss, in which nn means
the size of memory in *immovable* node, and ss means the start position of
this memory region. Then limit kaslr choose memory in these regions.

There are two policies:
1. Specify the memory region in *movable* node to avoid:
   Then we can use the existing mem_avoid to handle. But if the memory
   one movable node was separated by memory hole or different movable nodes
   are discontinuous, we don't know how many regions need to avoid.
   OTOH, we must avoid all of the movable memory, otherwise, kaslr may
   choose the wrong place.
2. Specify the memory region in "immovable* node to select:
   Only support 4 regions in this parameter. Then user can use two nodes
   at least for kaslr to choose, it's enough for the kernel to extract.
   At the same time, because we need only 4 new mem_vector, the usage
   of memory here is not too big.

PATCH 1/4 parse the extended movable_node=nn[KMG]@ss[KMG], then
	  store the memory regions.
PATCH 2/4 selects the memory region in immovable node when process
	  memmap.
PATCH 3/4 is the change of document.
PATCH 4/4 cleans up some little problems.

Chao Fan (4):
  kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]
  kaslr: select the memory region in immovable node to process
  document: change the document for the extended movable_node
  kaslr: clean up a useless variable and some usless space

 Documentation/admin-guide/kernel-parameters.txt |   9 ++
 arch/x86/boot/compressed/kaslr.c                | 140 +++++++++++++++++++++---
 2 files changed, 131 insertions(+), 18 deletions(-)

-- 
2.13.6

[PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]

[Chao Fan]

Extend the movable_node to movable_node=nn[KMG]@ss[KMG].
Since in current code, kaslr may choose the memory region in hot-pluggable
nodes. So we can specific the region in immovable node. And store the
regions in immovable_mem.

Multiple regions can be specified, comma delimited.
Considering the usage of memory, only support for 4 regions.
4 regions contains 2 nodes at least, and is enough for kernel to
extract.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 63 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 17818ba6906f..3c1f5204693b 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -107,6 +107,12 @@ enum mem_avoid_index {
 
 static struct mem_vector mem_avoid[MEM_AVOID_MAX];
 
+/* Only supporting at most 4 immovable memory regions with kaslr */
+#define MAX_IMMOVABLE_MEM	4
+
+static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
+static int num_immovable_region;
+
 static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
 {
 	/* Item one is entirely before item two. */
@@ -167,6 +173,28 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 	return -EINVAL;
 }
 
+static int parse_immovable_mem(char *p,
+			       unsigned long long *start,
+			       unsigned long long *size)
+{
+	char *oldp;
+
+	if (!p)
+		return -EINVAL;
+
+	oldp = p;
+	*size = memparse(p, &p);
+	if (p == oldp)
+		return -EINVAL;
+
+	if (*p == '@') {
+		*start = memparse(p + 1, &p);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static void mem_avoid_memmap(char *str)
 {
 	static int i;
@@ -206,6 +234,36 @@ static void mem_avoid_memmap(char *str)
 		memmap_too_large = true;
 }
 
+#ifdef CONFIG_MEMORY_HOTPLUG
+static void mem_mark_immovable(char *str)
+{
+	int i = 0;
+
+	while (str && (i < MAX_IMMOVABLE_MEM)) {
+		int rc;
+		unsigned long long start, size;
+		char *k = strchr(str, ',');
+
+		if (k)
+			*k++ = 0;
+
+		rc = parse_immovable_mem(str, &start, &size);
+		if (rc < 0)
+			break;
+		str = k;
+
+		immovable_mem[i].start = start;
+		immovable_mem[i].size = size;
+		i++;
+	}
+	num_immovable_region = i;
+}
+#else
+static inline void mem_mark_immovable(char *str)
+{
+}
+#endif
+
 static int handle_mem_memmap(void)
 {
 	char *args = (char *)get_cmd_line_ptr();
@@ -214,7 +272,8 @@ static int handle_mem_memmap(void)
 	char *param, *val;
 	u64 mem_size;
 
-	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
+	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
+	    !strstr(args, "movable_node="))
 		return 0;
 
 	tmp_cmdline = malloc(len + 1);
@@ -239,6 +298,8 @@ static int handle_mem_memmap(void)
 
 		if (!strcmp(param, "memmap")) {
 			mem_avoid_memmap(val);
+		} else if (!strcmp(param, "movable_node")) {
+			mem_mark_immovable(val);
 		} else if (!strcmp(param, "mem")) {
 			char *p = val;
 
-- 
2.13.6

[PATCH 2/4] kaslr: select the memory region in immovable node to process

[Chao Fan]

Since the interrelationship between e820 or efi entries and memory
region in immovable_mem is different:
One memory region in one node may contain several entries of e820 or
efi sometimes, and one entry of e820 or efi may contain the memory in
different nodes sometimes. So select the intersection as a region to
process_mem_region. It may split one node or one entry to several regions.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 72 ++++++++++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 14 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 3c1f5204693b..22330cbe8515 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -563,6 +563,7 @@ static void process_mem_region(struct mem_vector *entry,
 	end = min(entry->size + entry->start, mem_limit);
 	if (entry->start >= end)
 		return;
+
 	cur_entry.start = entry->start;
 	cur_entry.size = end - entry->start;
 
@@ -621,6 +622,52 @@ static void process_mem_region(struct mem_vector *entry,
 	}
 }
 
+static bool select_immovable_node(unsigned long long start,
+				  unsigned long long size,
+				  unsigned long long minimum,
+				  unsigned long long image_size)
+{
+	struct mem_vector region;
+	int i;
+
+	if (num_immovable_region == 0) {
+		region.start = start;
+		region.size = size;
+		process_mem_region(&region, minimum, image_size);
+
+		if (slot_area_index == MAX_SLOT_AREA) {
+			debug_putstr("Aborted memmap scan (slot_areas full)!\n");
+			return 1;
+		}
+	} else {
+		for (i = 0; i < num_immovable_region; i++) {
+			unsigned long long end, select_end;
+			unsigned long long region_start, region_end;
+
+			end = start + size - 1;
+			region_start = immovable_mem[i].start;
+			region_end = region_start + immovable_mem[i].size - 1;
+
+			if (end < region_start || start > region_end)
+				continue;
+
+			region.start = start > region_start ?
+				       start : region_start;
+			select_end = end > region_end ? region_end : end;
+
+			region.size = select_end - region.start + 1;
+
+			process_mem_region(&region, minimum, image_size);
+
+			if (slot_area_index == MAX_SLOT_AREA) {
+				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
+				return 1;
+			}
+		}
+	}
+	return 0;
+}
+
 #ifdef CONFIG_EFI
 /*
  * Returns true if mirror region found (and must have been processed
@@ -631,7 +678,6 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 {
 	struct efi_info *e = &boot_params->efi_info;
 	bool efi_mirror_found = false;
-	struct mem_vector region;
 	efi_memory_desc_t *md;
 	unsigned long pmap;
 	char *signature;
@@ -664,6 +710,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 	}
 
 	for (i = 0; i < nr_desc; i++) {
+		unsigned long long start, size;
+
 		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
 
 		/*
@@ -684,13 +732,11 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 		    !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
 			continue;
 
-		region.start = md->phys_addr;
-		region.size = md->num_pages << EFI_PAGE_SHIFT;
-		process_mem_region(&region, minimum, image_size);
-		if (slot_area_index == MAX_SLOT_AREA) {
-			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+		start = md->phys_addr;
+		size = md->num_pages << EFI_PAGE_SHIFT;
+
+		if (select_immovable_node(start, size, minimum, image_size))
 			break;
-		}
 	}
 	return true;
 }
@@ -706,22 +752,20 @@ static void process_e820_entries(unsigned long minimum,
 				 unsigned long image_size)
 {
 	int i;
-	struct mem_vector region;
 	struct boot_e820_entry *entry;
 
 	/* Verify potential e820 positions, appending to slots list. */
 	for (i = 0; i < boot_params->e820_entries; i++) {
+		unsigned long long start, size;
 		entry = &boot_params->e820_table[i];
 		/* Skip non-RAM entries. */
 		if (entry->type != E820_TYPE_RAM)
 			continue;
-		region.start = entry->addr;
-		region.size = entry->size;
-		process_mem_region(&region, minimum, image_size);
-		if (slot_area_index == MAX_SLOT_AREA) {
-			debug_putstr("Aborted e820 scan (slot_areas full)!\n");
+		start = entry->addr;
+		size = entry->size;
+
+		if (select_immovable_node(start, size, minimum, image_size))
 			break;
-		}
 	}
 }
 
-- 
2.13.6

[PATCH 3/4] document: change the document for the extended movable_node

[Chao Fan]

Add the document for the change of extended movable_node=nn[KMG]@ss[KMG].

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-doc@vger.kernel.org
Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index ead7f4066ea4..226560667d84 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2332,6 +2332,15 @@
 			allocations which rules out almost all kernel
 			allocations. Use with caution!
 
+	movable_node=nn[KMG]@ss[KMG]
+			[KNL] Force usage of a specific region of memory.
+			Extend movable_node to work well with KASLR.
+			Region of memory in immovable node is from ss to ss+nn.
+			Multiple regions can be specified, comma delimited.
+			Notice: we support 4 regions at most now.
+			Example:
+			movable_node=100M@2G,1G@4G
+
 	MTD_Partition=	[MTD]
 			Format: <name>,<region-number>,<size>,<offset>
 
-- 
2.13.6

[PATCH 4/4] kaslr: clean up a useless variable and some usless space

[Chao Fan]

There are two same variable "rc" in this function. One is in the
circulation, the other is out of the circulation. The one out will never
be used, so drop it.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 22330cbe8515..8a33ed82fd0b 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -198,7 +198,6 @@ static int parse_immovable_mem(char *p,
 static void mem_avoid_memmap(char *str)
 {
 	static int i;
-	int rc;
 
 	if (i >= MAX_MEMMAP_REGIONS)
 		return;
@@ -277,7 +276,7 @@ static int handle_mem_memmap(void)
 		return 0;
 
 	tmp_cmdline = malloc(len + 1);
-	if (!tmp_cmdline )
+	if (!tmp_cmdline)
 		error("Failed to allocate space for tmp_cmdline");
 
 	memcpy(tmp_cmdline, args, len);
@@ -423,7 +422,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	cmd_line |= boot_params->hdr.cmd_line_ptr;
 	/* Calculate size of cmd_line. */
 	ptr = (char *)(unsigned long)cmd_line;
-	for (cmd_line_size = 0; ptr[cmd_line_size++]; )
+	for (cmd_line_size = 0; ptr[cmd_line_size++];)
 		;
 	mem_avoid[MEM_AVOID_CMDLINE].start = cmd_line;
 	mem_avoid[MEM_AVOID_CMDLINE].size = cmd_line_size;
-- 
2.13.6

Re: [PATCH 0/4] kaslr: extend movable_node to movable_node=nn[KMG]@ss[KMG]

[Dou Liyang]

Hi Chao,

Cheer! I have some concerns below.

At 10/19/2017 06:02 PM, Chao Fan wrote:
> Here is a problem:
> Here is a machine with several NUMA nodes and some of them are hot-pluggable.
> It's not good for kernel to be extracted in the memory region of movable node.
> But in current code, I print the address choosen by kaslr and found it may be
> placed in movable node sometimes. To solve this problem, it's better to limit
> the memory region choosen by kaslr to immovable node in kaslr.c. But the memory
> infomation about if it's hot-pluggable is stored in ACPI SRAT table, which is
> parsed after kernel is extracted. So we can't get the detail memory infomation
> before extracting kernel.
>
> So extend the movable_node to movable_node=nn@ss, in which nn means
> the size of memory in *immovable* node, and ss means the start position of
> this memory region. Then limit kaslr choose memory in these regions.

Yes, great. Here we should remember that the situation of
'movable_node=nn@ss' is rare, normal situation is 'movable_node=nn'.

So, we should consider our code tendencies for normal situation. ;-)

>
> There are two policies:
> 1. Specify the memory region in *movable* node to avoid:
>    Then we can use the existing mem_avoid to handle. But if the memory
>    one movable node was separated by memory hole or different movable nodes
>    are discontinuous, we don't know how many regions need to avoid.

It is not a problem.

As you said, we should provide an interface for users later, like that:

# cat /sys/device/system/memory/movable_node
nn@ss


Thanks,
	dou.
>    OTOH, we must avoid all of the movable memory, otherwise, kaslr may
>    choose the wrong place.
> 2. Specify the memory region in "immovable* node to select:
>    Only support 4 regions in this parameter. Then user can use two nodes
>    at least for kaslr to choose, it's enough for the kernel to extract.
>    At the same time, because we need only 4 new mem_vector, the usage
>    of memory here is not too big.
>
> PATCH 1/4 parse the extended movable_node=nn[KMG]@ss[KMG], then
> 	  store the memory regions.
> PATCH 2/4 selects the memory region in immovable node when process
> 	  memmap.
> PATCH 3/4 is the change of document.
> PATCH 4/4 cleans up some little problems.
>
> Chao Fan (4):
>   kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]
>   kaslr: select the memory region in immovable node to process
>   document: change the document for the extended movable_node
>   kaslr: clean up a useless variable and some usless space
>
>  Documentation/admin-guide/kernel-parameters.txt |   9 ++
>  arch/x86/boot/compressed/kaslr.c                | 140 +++++++++++++++++++++---
>  2 files changed, 131 insertions(+), 18 deletions(-)
>

Re: [PATCH 0/4] kaslr: extend movable_node to movable_node=nn[KMG]@ss[KMG]

[Chao Fan]

On Fri, Oct 20, 2017 at 10:37:52AM +0800, Dou Liyang wrote:
>Hi Chao,
>
Hi Dou-san,

>Cheer! I have some concerns below.

Thanks for your reply.

>
>At 10/19/2017 06:02 PM, Chao Fan wrote:
>> Here is a problem:
>> Here is a machine with several NUMA nodes and some of them are hot-pluggable.
>> It's not good for kernel to be extracted in the memory region of movable node.
>> But in current code, I print the address choosen by kaslr and found it may be
>> placed in movable node sometimes. To solve this problem, it's better to limit
>> the memory region choosen by kaslr to immovable node in kaslr.c. But the memory
>> infomation about if it's hot-pluggable is stored in ACPI SRAT table, which is
>> parsed after kernel is extracted. So we can't get the detail memory infomation
>> before extracting kernel.
>> 
>> So extend the movable_node to movable_node=nn@ss, in which nn means
>> the size of memory in *immovable* node, and ss means the start position of
>> this memory region. Then limit kaslr choose memory in these regions.
>
>Yes, great. Here we should remember that the situation of
>'movable_node=nn@ss' is rare, normal situation is 'movable_node=nn'.
>
>So, we should consider our code tendencies for normal situation. ;-)

Yes, it's normal. But you can not make sure the special situation will
never happen,. If it happens, we can make sure codes work well, right?

We can not make sure that the movable nodes are continuous, or even if
the movable nodes are continuous, we can not make sure the memory
address are continuous.

It is easy to avoid the memory region in movable node.
But if we can handle more special situations, and at the same time,
make kernel more safe, why not?

>
>> 
>> There are two policies:
>> 1. Specify the memory region in *movable* node to avoid:
>>    Then we can use the existing mem_avoid to handle. But if the memory
>>    one movable node was separated by memory hole or different movable nodes
>>    are discontinuous, we don't know how many regions need to avoid.
>
>It is not a problem.
>
>As you said, we should provide an interface for users later, like that:
>
># cat /sys/device/system/memory/movable_node
>nn@ss
>

Both are OK. I think outputing the memory region in movable_node or
immovable_node are both reasonable. So the interface of both methods
will be useful. And after we decided which policy used in kaslr, then
add the interface of /sys.

Thanks,
Chao Fan

>
>Thanks,
>	dou.
>>    OTOH, we must avoid all of the movable memory, otherwise, kaslr may
>>    choose the wrong place.
>> 2. Specify the memory region in "immovable* node to select:
>>    Only support 4 regions in this parameter. Then user can use two nodes
>>    at least for kaslr to choose, it's enough for the kernel to extract.
>>    At the same time, because we need only 4 new mem_vector, the usage
>>    of memory here is not too big.
>> 
>> PATCH 1/4 parse the extended movable_node=nn[KMG]@ss[KMG], then
>> 	  store the memory regions.
>> PATCH 2/4 selects the memory region in immovable node when process
>> 	  memmap.
>> PATCH 3/4 is the change of document.
>> PATCH 4/4 cleans up some little problems.
>> 
>> Chao Fan (4):
>>   kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]
>>   kaslr: select the memory region in immovable node to process
>>   document: change the document for the extended movable_node
>>   kaslr: clean up a useless variable and some usless space
>> 
>>  Documentation/admin-guide/kernel-parameters.txt |   9 ++
>>  arch/x86/boot/compressed/kaslr.c                | 140 +++++++++++++++++++++---
>>  2 files changed, 131 insertions(+), 18 deletions(-)
>> 

Re: [PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]

[Dou Liyang]

Hi Chao,

At 10/19/2017 06:02 PM, Chao Fan wrote:
> Extend the movable_node to movable_node=nn[KMG]@ss[KMG].
> Since in current code, kaslr may choose the memory region in hot-pluggable
> nodes. So we can specific the region in immovable node. And store the
> regions in immovable_mem.
>

I guess you may mean that:

In current Linux with KASLR. Kernel may choose a memory region in
movable node for extracting kernel code, which will make the node
can't be hot-removed.

Solve it by only specific the region in immovable node. So create
immovable_mem to store the region of movable node, and only choose
the memory in immovable_mem array.

> Multiple regions can be specified, comma delimited.
> Considering the usage of memory, only support for 4 regions.
> 4 regions contains 2 nodes at least, and is enough for kernel to
> extract.
>
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 63 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 17818ba6906f..3c1f5204693b 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -107,6 +107,12 @@ enum mem_avoid_index {
>
>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>
> +/* Only supporting at most 4 immovable memory regions with kaslr */
> +#define MAX_IMMOVABLE_MEM	4
> +
> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
> +static int num_immovable_region;
> +
>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>  {
>  	/* Item one is entirely before item two. */
> @@ -167,6 +173,28 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>  	return -EINVAL;
>  }
>
> +static int parse_immovable_mem(char *p,
> +			       unsigned long long *start,
> +			       unsigned long long *size)
> +{
> +	char *oldp;
> +
> +	if (!p)
> +		return -EINVAL;
> +
> +	oldp = p;
> +	*size = memparse(p, &p);
> +	if (p == oldp)
> +		return -EINVAL;
> +
> +	if (*p == '@') {
> +		*start = memparse(p + 1, &p);
> +		return 0;
> +	}
> +

Here you don't consider that if @ss[KMG] is omitted.

> +	return -EINVAL;
> +}
> +
>  static void mem_avoid_memmap(char *str)
>  {
>  	static int i;
> @@ -206,6 +234,36 @@ static void mem_avoid_memmap(char *str)
>  		memmap_too_large = true;
>  }
>
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +static void mem_mark_immovable(char *str)
> +{
> +	int i = 0;
> +

you have use num_immovable_region, 'i' is useless. just remove it.

> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
> +		int rc;
> +		unsigned long long start, size;
> +		char *k = strchr(str, ',');
> +

Why do you put this definition here? IMO, moving it out is better.

> +		if (k)
> +			*k++ = 0;
> +
> +		rc = parse_immovable_mem(str, &start, &size);
> +		if (rc < 0)
> +			break;
> +		str = k;
> +
> +		immovable_mem[i].start = start;
> +		immovable_mem[i].size = size;
> +		i++;

Replace it with num_immovable_region

> +	}
> +	num_immovable_region = i;

Just remove it.

> +}
> +#else
> +static inline void mem_mark_immovable(char *str)
> +{
> +}
> +#endif
> +
>  static int handle_mem_memmap(void)
>  {
>  	char *args = (char *)get_cmd_line_ptr();
> @@ -214,7 +272,8 @@ static int handle_mem_memmap(void)
>  	char *param, *val;
>  	u64 mem_size;
>
> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> +	    !strstr(args, "movable_node="))
>  		return 0;
>
>  	tmp_cmdline = malloc(len + 1);
> @@ -239,6 +298,8 @@ static int handle_mem_memmap(void)
>
>  		if (!strcmp(param, "memmap")) {
>  			mem_avoid_memmap(val);
> +		} else if (!strcmp(param, "movable_node")) {
> +			mem_mark_immovable(val);

AFAIK, handle_mem_memmap() is invoked in mem_avoid_init(), which is used 
to avoid mem. But, here the value of immovable node is the memory
you want to mark and use, it is better that we split it out.

BTW, Using movable_node to store the memory of immovable node is
strange and make me confuse. How about adding a new command option.

Thanks,
	dou.
>  		} else if (!strcmp(param, "mem")) {
>  			char *p = val;
>
>

Re: [PATCH 2/4] kaslr: select the memory region in immovable node to process

[Dou Liyang]

Hi Chao

At 10/19/2017 06:02 PM, Chao Fan wrote:
> Since the interrelationship between e820 or efi entries and memory
> region in immovable_mem is different:
> One memory region in one node may contain several entries of e820 or
> efi sometimes, and one entry of e820 or efi may contain the memory in
> different nodes sometimes. So select the intersection as a region to
> process_mem_region. It may split one node or one entry to several regions.
>
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 72 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 3c1f5204693b..22330cbe8515 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -563,6 +563,7 @@ static void process_mem_region(struct mem_vector *entry,
>  	end = min(entry->size + entry->start, mem_limit);
>  	if (entry->start >= end)
>  		return;
> +
>  	cur_entry.start = entry->start;
>  	cur_entry.size = end - entry->start;

Above code has nothing to do with this patch. remove it.

>
> @@ -621,6 +622,52 @@ static void process_mem_region(struct mem_vector *entry,
>  	}
>  }
>
> +static bool select_immovable_node(unsigned long long start,
> +				  unsigned long long size,
> +				  unsigned long long minimum,
> +				  unsigned long long image_size)
> +{
> +	struct mem_vector region;
> +	int i;
> +
> +	if (num_immovable_region == 0) {

Seems it more better:

#ifdef CONFIG_MEMORY_HOTPLUG
for (i = 0; i < num_immovable_region; i++) {
   ...
}
#else
...
process_mem_region(&region, minimum, image_size);
...
#endif

> +		region.start = start;
> +		region.size = size;
> +		process_mem_region(&region, minimum, image_size);
> +
> +		if (slot_area_index == MAX_SLOT_AREA) {
> +			debug_putstr("Aborted memmap scan (slot_areas full)!\n");
> +			return 1;
> +		}
> +	} else {
> +		for (i = 0; i < num_immovable_region; i++) {
> +			unsigned long long end, select_end;
> +			unsigned long long region_start, region_end;
> +
> +			end = start + size - 1;
> +			region_start = immovable_mem[i].start;
> +			region_end = region_start + immovable_mem[i].size - 1;
> +
> +			if (end < region_start || start > region_end)
> +				continue;
> +
> +			region.start = start > region_start ?
> +				       start : region_start;
> +			select_end = end > region_end ? region_end : end;
> +
> +			region.size = select_end - region.start + 1;
> +
> +			process_mem_region(&region, minimum, image_size);
> +
> +			if (slot_area_index == MAX_SLOT_AREA) {
> +				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
> +				return 1;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
>  #ifdef CONFIG_EFI
>  /*
>   * Returns true if mirror region found (and must have been processed
> @@ -631,7 +678,6 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>  {
>  	struct efi_info *e = &boot_params->efi_info;
>  	bool efi_mirror_found = false;
> -	struct mem_vector region;
>  	efi_memory_desc_t *md;
>  	unsigned long pmap;
>  	char *signature;
> @@ -664,6 +710,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>  	}
>
>  	for (i = 0; i < nr_desc; i++) {
> +		unsigned long long start, size;
> +
>  		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
>
>  		/*
> @@ -684,13 +732,11 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>  		    !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
>  			continue;
>
> -		region.start = md->phys_addr;
> -		region.size = md->num_pages << EFI_PAGE_SHIFT;
> -		process_mem_region(&region, minimum, image_size);
> -		if (slot_area_index == MAX_SLOT_AREA) {
> -			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
> +		start = md->phys_addr;
> +		size = md->num_pages << EFI_PAGE_SHIFT;
> +
> +		if (select_immovable_node(start, size, minimum, image_size))

Why you replace the region with two parameters? Just use region.

>  			break;
> -		}
>  	}
>  	return true;
>  }
> @@ -706,22 +752,20 @@ static void process_e820_entries(unsigned long minimum,
>  				 unsigned long image_size)
>  {
>  	int i;
> -	struct mem_vector region;
>  	struct boot_e820_entry *entry;
>
>  	/* Verify potential e820 positions, appending to slots list. */
>  	for (i = 0; i < boot_params->e820_entries; i++) {
> +		unsigned long long start, size;
>  		entry = &boot_params->e820_table[i];
>  		/* Skip non-RAM entries. */
>  		if (entry->type != E820_TYPE_RAM)
>  			continue;
> -		region.start = entry->addr;
> -		region.size = entry->size;
> -		process_mem_region(&region, minimum, image_size);
> -		if (slot_area_index == MAX_SLOT_AREA) {
> -			debug_putstr("Aborted e820 scan (slot_areas full)!\n");
> +		start = entry->addr;
> +		size = entry->size;
> +
> +		if (select_immovable_node(start, size, minimum, image_size))
ditto

Thanks,
	dou.
>  			break;
> -		}
>  	}
>  }
>
>

Re: [PATCH 4/4] kaslr: clean up a useless variable and some usless space

[Dou Liyang]

Hi Chao,

At 10/19/2017 06:02 PM, Chao Fan wrote:
> There are two same variable "rc" in this function. One is in the
> circulation, the other is out of the circulation. The one out will never
> be used, so drop it.
>
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 22330cbe8515..8a33ed82fd0b 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -198,7 +198,6 @@ static int parse_immovable_mem(char *p,
>  static void mem_avoid_memmap(char *str)
>  {
>  	static int i;
> -	int rc;
>
>  	if (i >= MAX_MEMMAP_REGIONS)
>  		return;

Seems it is redundant too,

Thanks,
	dou.

> @@ -277,7 +276,7 @@ static int handle_mem_memmap(void)
>  		return 0;
>
>  	tmp_cmdline = malloc(len + 1);
> -	if (!tmp_cmdline )
> +	if (!tmp_cmdline)
>  		error("Failed to allocate space for tmp_cmdline");
>
>  	memcpy(tmp_cmdline, args, len);
> @@ -423,7 +422,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>  	cmd_line |= boot_params->hdr.cmd_line_ptr;
>  	/* Calculate size of cmd_line. */
>  	ptr = (char *)(unsigned long)cmd_line;
> -	for (cmd_line_size = 0; ptr[cmd_line_size++]; )
> +	for (cmd_line_size = 0; ptr[cmd_line_size++];)
>  		;
>  	mem_avoid[MEM_AVOID_CMDLINE].start = cmd_line;
>  	mem_avoid[MEM_AVOID_CMDLINE].size = cmd_line_size;
>

Re: [PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]

[Chao Fan]

On Fri, Oct 20, 2017 at 11:04:42AM +0800, Dou Liyang wrote:
>Hi Chao,
>
Hi Dou-san,

>At 10/19/2017 06:02 PM, Chao Fan wrote:
>> Extend the movable_node to movable_node=nn[KMG]@ss[KMG].
>> Since in current code, kaslr may choose the memory region in hot-pluggable
>> nodes. So we can specific the region in immovable node. And store the
>> regions in immovable_mem.
>> 
>
>I guess you may mean that:
>
>In current Linux with KASLR. Kernel may choose a memory region in
>movable node for extracting kernel code, which will make the node
>can't be hot-removed.
>
>Solve it by only specific the region in immovable node. So create
>immovable_mem to store the region of movable node, and only choose
>the memory in immovable_mem array.
>

Thanks for the explaination.

>> Multiple regions can be specified, comma delimited.
>> Considering the usage of memory, only support for 4 regions.
>> 4 regions contains 2 nodes at least, and is enough for kernel to
>> extract.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 63 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 62 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 17818ba6906f..3c1f5204693b 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -107,6 +107,12 @@ enum mem_avoid_index {
>> 
>>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>> 
>> +/* Only supporting at most 4 immovable memory regions with kaslr */
>> +#define MAX_IMMOVABLE_MEM	4
>> +
>> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>> +static int num_immovable_region;
>> +
>>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>>  {
>>  	/* Item one is entirely before item two. */
>> @@ -167,6 +173,28 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>>  	return -EINVAL;
>>  }
>> 
>> +static int parse_immovable_mem(char *p,
>> +			       unsigned long long *start,
>> +			       unsigned long long *size)
>> +{
>> +	char *oldp;
>> +
>> +	if (!p)
>> +		return -EINVAL;
>> +
>> +	oldp = p;
>> +	*size = memparse(p, &p);
>> +	if (p == oldp)
>> +		return -EINVAL;
>> +
>> +	if (*p == '@') {
>> +		*start = memparse(p + 1, &p);
>> +		return 0;
>> +	}
>> +
>
>Here you don't consider that if @ss[KMG] is omitted.

Yes, will add. Many thanks.

>
>> +	return -EINVAL;
>> +}
>> +
>>  static void mem_avoid_memmap(char *str)
>>  {
>>  	static int i;
>> @@ -206,6 +234,36 @@ static void mem_avoid_memmap(char *str)
>>  		memmap_too_large = true;
>>  }
>> 
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +static void mem_mark_immovable(char *str)
>> +{
>> +	int i = 0;
>> +
>
>you have use num_immovable_region, 'i' is useless. just remove it.

Using num_immovable_region makes code too long. Using i will be
clear and make sure shoter than 80 characters.

>
>> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
>> +		int rc;
>> +		unsigned long long start, size;
>> +		char *k = strchr(str, ',');
>> +
>
>Why do you put this definition here? IMO, moving it out is better.
>
>> +		if (k)
>> +			*k++ = 0;
>> +
>> +		rc = parse_immovable_mem(str, &start, &size);
>> +		if (rc < 0)
>> +			break;
>> +		str = k;
>> +
>> +		immovable_mem[i].start = start;
>> +		immovable_mem[i].size = size;
>> +		i++;
>
>Replace it with num_immovable_region

ditto...
Why do you care this little variable so much.

>
>> +	}
>> +	num_immovable_region = i;
>
>Just remove it.

ditto.

>
>> +}
>> +#else
>> +static inline void mem_mark_immovable(char *str)
>> +{
>> +}
>> +#endif
>> +
>>  static int handle_mem_memmap(void)
>>  {
>>  	char *args = (char *)get_cmd_line_ptr();
>> @@ -214,7 +272,8 @@ static int handle_mem_memmap(void)
>>  	char *param, *val;
>>  	u64 mem_size;
>> 
>> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>> +	    !strstr(args, "movable_node="))
>>  		return 0;
>> 
>>  	tmp_cmdline = malloc(len + 1);
>> @@ -239,6 +298,8 @@ static int handle_mem_memmap(void)
>> 
>>  		if (!strcmp(param, "memmap")) {
>>  			mem_avoid_memmap(val);
>> +		} else if (!strcmp(param, "movable_node")) {
>> +			mem_mark_immovable(val);
>
>AFAIK, handle_mem_memmap() is invoked in mem_avoid_init(), which is used to
>avoid mem. But, here the value of immovable node is the memory
>you want to mark and use, it is better that we split it out.

There is existing and useful code, so it's better to reuse but not
re-write.

>
>BTW, Using movable_node to store the memory of immovable node is
>strange and make me confuse. How about adding a new command option.
>

I have added the document, if you think there are problems, please let
me know.

Thanks,
Chao Fan

>Thanks,
>	dou.
>>  		} else if (!strcmp(param, "mem")) {
>>  			char *p = val;
>> 
>> 

Re: [PATCH 0/4] kaslr: extend movable_node to movable_node=nn[KMG]@ss[KMG]

[Dou Liyang]

Hi Chao,

At 10/20/2017 10:53 AM, Chao Fan wrote:
> On Fri, Oct 20, 2017 at 10:37:52AM +0800, Dou Liyang wrote:
>> Hi Chao,
>>
> Hi Dou-san,
>
>> Cheer! I have some concerns below.
>
> Thanks for your reply.
>
>>
>> At 10/19/2017 06:02 PM, Chao Fan wrote:
>>> Here is a problem:
>>> Here is a machine with several NUMA nodes and some of them are hot-pluggable.
>>> It's not good for kernel to be extracted in the memory region of movable node.
>>> But in current code, I print the address choosen by kaslr and found it may be
>>> placed in movable node sometimes. To solve this problem, it's better to limit
>>> the memory region choosen by kaslr to immovable node in kaslr.c. But the memory
>>> infomation about if it's hot-pluggable is stored in ACPI SRAT table, which is
>>> parsed after kernel is extracted. So we can't get the detail memory infomation
>>> before extracting kernel.
>>>
>>> So extend the movable_node to movable_node=nn@ss, in which nn means
>>> the size of memory in *immovable* node, and ss means the start position of
>>> this memory region. Then limit kaslr choose memory in these regions.
>>
>> Yes, great. Here we should remember that the situation of
>> 'movable_node=nn@ss' is rare, normal situation is 'movable_node=nn'.
>>
>> So, we should consider our code tendencies for normal situation. ;-)
>
> Yes, it's normal. But you can not make sure the special situation will
> never happen,. If it happens, we can make sure codes work well, right?
>
> We can not make sure that the movable nodes are continuous, or even if
> the movable nodes are continuous, we can not make sure the memory
> address are continuous.
>
> It is easy to avoid the memory region in movable node.
> But if we can handle more special situations, and at the same time,
> make kernel more safe, why not?

You misunderstand my opinions, I means that
when we code, we need to know the problem clearly and which part of
problem will often be executed.

Make our code more suitable for the normal situation without affecting 
the function of the problem.
Just like:

likely() and unlikely()

Here I guess you don't consider that. so I said that.

>
>>
>>>
>>> There are two policies:
>>> 1. Specify the memory region in *movable* node to avoid:
>>>    Then we can use the existing mem_avoid to handle. But if the memory
>>>    one movable node was separated by memory hole or different movable nodes
>>>    are discontinuous, we don't know how many regions need to avoid.
>>
>> It is not a problem.
>>
>> As you said, we should provide an interface for users later, like that:
>>
>> # cat /sys/device/system/memory/movable_node
>> nn@ss
>>
>
> Both are OK. I think outputing the memory region in movable_node or
> immovable_node are both reasonable. So the interface of both methods
> will be useful. And after we decided which policy used in kaslr, then
> add the interface of /sys.
>

Actually, I prefer the first one, are you ready to post the patches
for the first policy?

Thanks,
	dou.
> Thanks,
> Chao Fan
>
>>
>> Thanks,
>> 	dou.
>>>    OTOH, we must avoid all of the movable memory, otherwise, kaslr may
>>>    choose the wrong place.
>>> 2. Specify the memory region in "immovable* node to select:
>>>    Only support 4 regions in this parameter. Then user can use two nodes
>>>    at least for kaslr to choose, it's enough for the kernel to extract.
>>>    At the same time, because we need only 4 new mem_vector, the usage
>>>    of memory here is not too big.
>>>
>>> PATCH 1/4 parse the extended movable_node=nn[KMG]@ss[KMG], then
>>> 	  store the memory regions.
>>> PATCH 2/4 selects the memory region in immovable node when process
>>> 	  memmap.
>>> PATCH 3/4 is the change of document.
>>> PATCH 4/4 cleans up some little problems.
>>>
>>> Chao Fan (4):
>>>   kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]
>>>   kaslr: select the memory region in immovable node to process
>>>   document: change the document for the extended movable_node
>>>   kaslr: clean up a useless variable and some usless space
>>>
>>>  Documentation/admin-guide/kernel-parameters.txt |   9 ++
>>>  arch/x86/boot/compressed/kaslr.c                | 140 +++++++++++++++++++++---
>>>  2 files changed, 131 insertions(+), 18 deletions(-)
>>>
>

Re: [PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]

[Dou Liyang]

At 10/20/2017 11:22 AM, Chao Fan wrote:
> On Fri, Oct 20, 2017 at 11:04:42AM +0800, Dou Liyang wrote:
>> Hi Chao,
>>
> Hi Dou-san,
>
>> At 10/19/2017 06:02 PM, Chao Fan wrote:
>>> Extend the movable_node to movable_node=nn[KMG]@ss[KMG].
>>> Since in current code, kaslr may choose the memory region in hot-pluggable
>>> nodes. So we can specific the region in immovable node. And store the
>>> regions in immovable_mem.
>>>
>>
>> I guess you may mean that:
>>
>> In current Linux with KASLR. Kernel may choose a memory region in
>> movable node for extracting kernel code, which will make the node
>> can't be hot-removed.
>>
>> Solve it by only specific the region in immovable node. So create
>> immovable_mem to store the region of movable node, and only choose
>> the memory in immovable_mem array.
>>
>
> Thanks for the explaination.
>
>>> Multiple regions can be specified, comma delimited.
>>> Considering the usage of memory, only support for 4 regions.
>>> 4 regions contains 2 nodes at least, and is enough for kernel to
>>> extract.
>>>
>>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>>> ---
>>>  arch/x86/boot/compressed/kaslr.c | 63 +++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 62 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>>> index 17818ba6906f..3c1f5204693b 100644
>>> --- a/arch/x86/boot/compressed/kaslr.c
>>> +++ b/arch/x86/boot/compressed/kaslr.c
>>> @@ -107,6 +107,12 @@ enum mem_avoid_index {
>>>
>>>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>>>
>>> +/* Only supporting at most 4 immovable memory regions with kaslr */
>>> +#define MAX_IMMOVABLE_MEM	4
>>> +
>>> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>>> +static int num_immovable_region;
>>> +
>>>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>>>  {
>>>  	/* Item one is entirely before item two. */
>>> @@ -167,6 +173,28 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>>>  	return -EINVAL;
>>>  }
>>>
>>> +static int parse_immovable_mem(char *p,
>>> +			       unsigned long long *start,
>>> +			       unsigned long long *size)
>>> +{
>>> +	char *oldp;
>>> +
>>> +	if (!p)
>>> +		return -EINVAL;
>>> +
>>> +	oldp = p;
>>> +	*size = memparse(p, &p);
>>> +	if (p == oldp)
>>> +		return -EINVAL;
>>> +
>>> +	if (*p == '@') {
>>> +		*start = memparse(p + 1, &p);
>>> +		return 0;
>>> +	}
>>> +
>>
>> Here you don't consider that if @ss[KMG] is omitted.
>
> Yes, will add. Many thanks.
>
>>
>>> +	return -EINVAL;
>>> +}
>>> +
>>>  static void mem_avoid_memmap(char *str)
>>>  {
>>>  	static int i;
>>> @@ -206,6 +234,36 @@ static void mem_avoid_memmap(char *str)
>>>  		memmap_too_large = true;
>>>  }
>>>
>>> +#ifdef CONFIG_MEMORY_HOTPLUG
>>> +static void mem_mark_immovable(char *str)
>>> +{
>>> +	int i = 0;
>>> +
>>
>> you have use num_immovable_region, 'i' is useless. just remove it.
>
> Using num_immovable_region makes code too long. Using i will be
> clear and make sure shoter than 80 characters.

Oh, God, that's horrific. Did you find that your code is wrong?

num_immovable_region will be reset each time.

Thanks,
	dou.

>
>>
>>> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
>>> +		int rc;
>>> +		unsigned long long start, size;
>>> +		char *k = strchr(str, ',');
>>> +
>>
>> Why do you put this definition here? IMO, moving it out is better.
>>
>>> +		if (k)
>>> +			*k++ = 0;
>>> +
>>> +		rc = parse_immovable_mem(str, &start, &size);
>>> +		if (rc < 0)
>>> +			break;
>>> +		str = k;
>>> +
>>> +		immovable_mem[i].start = start;
>>> +		immovable_mem[i].size = size;
>>> +		i++;
>>
>> Replace it with num_immovable_region
>
> ditto...
> Why do you care this little variable so much.
>
>>
>>> +	}
>>> +	num_immovable_region = i;
>>
>> Just remove it.
>
> ditto.
>
>>
>>> +}
>>> +#else
>>> +static inline void mem_mark_immovable(char *str)
>>> +{
>>> +}
>>> +#endif
>>> +
>>>  static int handle_mem_memmap(void)
>>>  {
>>>  	char *args = (char *)get_cmd_line_ptr();
>>> @@ -214,7 +272,8 @@ static int handle_mem_memmap(void)
>>>  	char *param, *val;
>>>  	u64 mem_size;
>>>
>>> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>>> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>>> +	    !strstr(args, "movable_node="))
>>>  		return 0;
>>>
>>>  	tmp_cmdline = malloc(len + 1);
>>> @@ -239,6 +298,8 @@ static int handle_mem_memmap(void)
>>>
>>>  		if (!strcmp(param, "memmap")) {
>>>  			mem_avoid_memmap(val);
>>> +		} else if (!strcmp(param, "movable_node")) {
>>> +			mem_mark_immovable(val);
>>
>> AFAIK, handle_mem_memmap() is invoked in mem_avoid_init(), which is used to
>> avoid mem. But, here the value of immovable node is the memory
>> you want to mark and use, it is better that we split it out.
>
> There is existing and useful code, so it's better to reuse but not
> re-write.
>
>>
>> BTW, Using movable_node to store the memory of immovable node is
>> strange and make me confuse. How about adding a new command option.
>>
>
> I have added the document, if you think there are problems, please let
> me know.
>
> Thanks,
> Chao Fan
>
>> Thanks,
>> 	dou.
>>>  		} else if (!strcmp(param, "mem")) {
>>>  			char *p = val;
>>>
>>>
>

Re: [PATCH 2/4] kaslr: select the memory region in immovable node to process

[Chao Fan]

On Fri, Oct 20, 2017 at 11:17:26AM +0800, Dou Liyang wrote:
>Hi Chao
>
Hi Dou-san,

>At 10/19/2017 06:02 PM, Chao Fan wrote:
>> Since the interrelationship between e820 or efi entries and memory
>> region in immovable_mem is different:
>> One memory region in one node may contain several entries of e820 or
>> efi sometimes, and one entry of e820 or efi may contain the memory in
>> different nodes sometimes. So select the intersection as a region to
>> process_mem_region. It may split one node or one entry to several regions.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 72 ++++++++++++++++++++++++++++++++--------
>>  1 file changed, 58 insertions(+), 14 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 3c1f5204693b..22330cbe8515 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -563,6 +563,7 @@ static void process_mem_region(struct mem_vector *entry,
>>  	end = min(entry->size + entry->start, mem_limit);
>>  	if (entry->start >= end)
>>  		return;
>> +
>>  	cur_entry.start = entry->start;
>>  	cur_entry.size = end - entry->start;
>
>Above code has nothing to do with this patch. remove it.
>
>> 
>> @@ -621,6 +622,52 @@ static void process_mem_region(struct mem_vector *entry,
>>  	}
>>  }
>> 
>> +static bool select_immovable_node(unsigned long long start,
>> +				  unsigned long long size,
>> +				  unsigned long long minimum,
>> +				  unsigned long long image_size)
>> +{
>> +	struct mem_vector region;
>> +	int i;
>> +
>> +	if (num_immovable_region == 0) {
>
>Seems it more better:
>
>#ifdef CONFIG_MEMORY_HOTPLUG
>for (i = 0; i < num_immovable_region; i++) {
>  ...
>}
>#else
>...
>process_mem_region(&region, minimum, image_size);
>...
>#endif

No, if we set CONFIG_MEMORY_HOTPLUG=y(the default is y in fedora), but we do not
use movable_node, we should go to process_mem_region straight, right?
But in your change, we still go to the for(...), even if
num_immovable_region == 0.
So if num_immovable_region is 0, it includes the situation of
CONFIG_MEMORY_HOTPLUG=n

>
>> +		region.start = start;
>> +		region.size = size;
>> +		process_mem_region(&region, minimum, image_size);
>> +
>> +		if (slot_area_index == MAX_SLOT_AREA) {
>> +			debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>> +			return 1;
>> +		}
>> +	} else {
>> +		for (i = 0; i < num_immovable_region; i++) {
>> +			unsigned long long end, select_end;
>> +			unsigned long long region_start, region_end;
>> +
>> +			end = start + size - 1;
>> +			region_start = immovable_mem[i].start;
>> +			region_end = region_start + immovable_mem[i].size - 1;
>> +
>> +			if (end < region_start || start > region_end)
>> +				continue;
>> +
>> +			region.start = start > region_start ?
>> +				       start : region_start;
>> +			select_end = end > region_end ? region_end : end;
>> +
>> +			region.size = select_end - region.start + 1;
>> +
>> +			process_mem_region(&region, minimum, image_size);
>> +
>> +			if (slot_area_index == MAX_SLOT_AREA) {
>> +				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>> +				return 1;
>> +			}
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>>  #ifdef CONFIG_EFI
>>  /*
>>   * Returns true if mirror region found (and must have been processed
>> @@ -631,7 +678,6 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>>  {
>>  	struct efi_info *e = &boot_params->efi_info;
>>  	bool efi_mirror_found = false;
>> -	struct mem_vector region;
>>  	efi_memory_desc_t *md;
>>  	unsigned long pmap;
>>  	char *signature;
>> @@ -664,6 +710,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>>  	}
>> 
>>  	for (i = 0; i < nr_desc; i++) {
>> +		unsigned long long start, size;
>> +
>>  		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
>> 
>>  		/*
>> @@ -684,13 +732,11 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>>  		    !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
>>  			continue;
>> 
>> -		region.start = md->phys_addr;
>> -		region.size = md->num_pages << EFI_PAGE_SHIFT;
>> -		process_mem_region(&region, minimum, image_size);
>> -		if (slot_area_index == MAX_SLOT_AREA) {
>> -			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
>> +		start = md->phys_addr;
>> +		size = md->num_pages << EFI_PAGE_SHIFT;
>> +
>> +		if (select_immovable_node(start, size, minimum, image_size))
>
>Why you replace the region with two parameters? Just use region.
>

You can see the commit message, we may splite one entry to more regions,
so there will be region.start compared with memory region in
immovable_mem and then fill more new regions if we use region here.
The code will look strange.

Start and size will be more clear.

Thanks,
Chao Fan

>>  			break;
>> -		}
>>  	}
>>  	return true;
>>  }
>> @@ -706,22 +752,20 @@ static void process_e820_entries(unsigned long minimum,
>>  				 unsigned long image_size)
>>  {
>>  	int i;
>> -	struct mem_vector region;
>>  	struct boot_e820_entry *entry;
>> 
>>  	/* Verify potential e820 positions, appending to slots list. */
>>  	for (i = 0; i < boot_params->e820_entries; i++) {
>> +		unsigned long long start, size;
>>  		entry = &boot_params->e820_table[i];
>>  		/* Skip non-RAM entries. */
>>  		if (entry->type != E820_TYPE_RAM)
>>  			continue;
>> -		region.start = entry->addr;
>> -		region.size = entry->size;
>> -		process_mem_region(&region, minimum, image_size);
>> -		if (slot_area_index == MAX_SLOT_AREA) {
>> -			debug_putstr("Aborted e820 scan (slot_areas full)!\n");
>> +		start = entry->addr;
>> +		size = entry->size;
>> +
>> +		if (select_immovable_node(start, size, minimum, image_size))
>ditto
>
>Thanks,
>	dou.
>>  			break;
>> -		}
>>  	}
>>  }
>> 
>> 

Re: [PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]

[Chao Fan]

On Fri, Oct 20, 2017 at 11:41:19AM +0800, Dou Liyang wrote:
>
>
>At 10/20/2017 11:22 AM, Chao Fan wrote:
>> On Fri, Oct 20, 2017 at 11:04:42AM +0800, Dou Liyang wrote:
>> > Hi Chao,
>> > 
>> Hi Dou-san,
>> 
>> > At 10/19/2017 06:02 PM, Chao Fan wrote:
>> > > Extend the movable_node to movable_node=nn[KMG]@ss[KMG].
>> > > Since in current code, kaslr may choose the memory region in hot-pluggable
>> > > nodes. So we can specific the region in immovable node. And store the
>> > > regions in immovable_mem.
>> > > 
>> > 
>> > I guess you may mean that:
>> > 
>> > In current Linux with KASLR. Kernel may choose a memory region in
>> > movable node for extracting kernel code, which will make the node
>> > can't be hot-removed.
>> > 
>> > Solve it by only specific the region in immovable node. So create
>> > immovable_mem to store the region of movable node, and only choose
>> > the memory in immovable_mem array.
>> > 
>> 
>> Thanks for the explaination.
>> 
>> > > Multiple regions can be specified, comma delimited.
>> > > Considering the usage of memory, only support for 4 regions.
>> > > 4 regions contains 2 nodes at least, and is enough for kernel to
>> > > extract.
>> > > 
>> > > Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> > > ---
>> > >  arch/x86/boot/compressed/kaslr.c | 63 +++++++++++++++++++++++++++++++++++++++-
>> > >  1 file changed, 62 insertions(+), 1 deletion(-)
>> > > 
>> > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> > > index 17818ba6906f..3c1f5204693b 100644
>> > > --- a/arch/x86/boot/compressed/kaslr.c
>> > > +++ b/arch/x86/boot/compressed/kaslr.c
>> > > @@ -107,6 +107,12 @@ enum mem_avoid_index {
>> > > 
>> > >  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>> > > 
>> > > +/* Only supporting at most 4 immovable memory regions with kaslr */
>> > > +#define MAX_IMMOVABLE_MEM	4
>> > > +
>> > > +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>> > > +static int num_immovable_region;
>> > > +
>> > >  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>> > >  {
>> > >  	/* Item one is entirely before item two. */
>> > > @@ -167,6 +173,28 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>> > >  	return -EINVAL;
>> > >  }
>> > > 
>> > > +static int parse_immovable_mem(char *p,
>> > > +			       unsigned long long *start,
>> > > +			       unsigned long long *size)
>> > > +{
>> > > +	char *oldp;
>> > > +
>> > > +	if (!p)
>> > > +		return -EINVAL;
>> > > +
>> > > +	oldp = p;
>> > > +	*size = memparse(p, &p);
>> > > +	if (p == oldp)
>> > > +		return -EINVAL;
>> > > +
>> > > +	if (*p == '@') {
>> > > +		*start = memparse(p + 1, &p);
>> > > +		return 0;
>> > > +	}
>> > > +
>> > 
>> > Here you don't consider that if @ss[KMG] is omitted.
>> 
>> Yes, will add. Many thanks.
>> 
>> > 
>> > > +	return -EINVAL;
>> > > +}
>> > > +
>> > >  static void mem_avoid_memmap(char *str)
>> > >  {
>> > >  	static int i;
>> > > @@ -206,6 +234,36 @@ static void mem_avoid_memmap(char *str)
>> > >  		memmap_too_large = true;
>> > >  }
>> > > 
>> > > +#ifdef CONFIG_MEMORY_HOTPLUG
>> > > +static void mem_mark_immovable(char *str)
>> > > +{
>> > > +	int i = 0;
>> > > +
>> > 
>> > you have use num_immovable_region, 'i' is useless. just remove it.
>> 
>> Using num_immovable_region makes code too long. Using i will be
>> clear and make sure shoter than 80 characters.
>
>Oh, God, that's horrific. Did you find that your code is wrong?
>
>num_immovable_region will be reset each time.

Did you test?

Thanks,
Chao Fan

>
>Thanks,
>	dou.
>
>> 
>> > 
>> > > +	while (str && (i < MAX_IMMOVABLE_MEM)) {
>> > > +		int rc;
>> > > +		unsigned long long start, size;
>> > > +		char *k = strchr(str, ',');
>> > > +
>> > 
>> > Why do you put this definition here? IMO, moving it out is better.
>> > 
>> > > +		if (k)
>> > > +			*k++ = 0;
>> > > +
>> > > +		rc = parse_immovable_mem(str, &start, &size);
>> > > +		if (rc < 0)
>> > > +			break;
>> > > +		str = k;
>> > > +
>> > > +		immovable_mem[i].start = start;
>> > > +		immovable_mem[i].size = size;
>> > > +		i++;
>> > 
>> > Replace it with num_immovable_region
>> 
>> ditto...
>> Why do you care this little variable so much.
>> 
>> > 
>> > > +	}
>> > > +	num_immovable_region = i;
>> > 
>> > Just remove it.
>> 
>> ditto.
>> 
>> > 
>> > > +}
>> > > +#else
>> > > +static inline void mem_mark_immovable(char *str)
>> > > +{
>> > > +}
>> > > +#endif
>> > > +
>> > >  static int handle_mem_memmap(void)
>> > >  {
>> > >  	char *args = (char *)get_cmd_line_ptr();
>> > > @@ -214,7 +272,8 @@ static int handle_mem_memmap(void)
>> > >  	char *param, *val;
>> > >  	u64 mem_size;
>> > > 
>> > > -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>> > > +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>> > > +	    !strstr(args, "movable_node="))
>> > >  		return 0;
>> > > 
>> > >  	tmp_cmdline = malloc(len + 1);
>> > > @@ -239,6 +298,8 @@ static int handle_mem_memmap(void)
>> > > 
>> > >  		if (!strcmp(param, "memmap")) {
>> > >  			mem_avoid_memmap(val);
>> > > +		} else if (!strcmp(param, "movable_node")) {
>> > > +			mem_mark_immovable(val);
>> > 
>> > AFAIK, handle_mem_memmap() is invoked in mem_avoid_init(), which is used to
>> > avoid mem. But, here the value of immovable node is the memory
>> > you want to mark and use, it is better that we split it out.
>> 
>> There is existing and useful code, so it's better to reuse but not
>> re-write.
>> 
>> > 
>> > BTW, Using movable_node to store the memory of immovable node is
>> > strange and make me confuse. How about adding a new command option.
>> > 
>> 
>> I have added the document, if you think there are problems, please let
>> me know.
>> 
>> Thanks,
>> Chao Fan
>> 
>> > Thanks,
>> > 	dou.
>> > >  		} else if (!strcmp(param, "mem")) {
>> > >  			char *p = val;
>> > > 
>> > > 
>> 

Re: [PATCH 0/4] kaslr: extend movable_node to movable_node=nn[KMG]@ss[KMG]

[Chao Fan]

On Fri, Oct 20, 2017 at 11:37:11AM +0800, Dou Liyang wrote:
>Hi Chao,
>
>At 10/20/2017 10:53 AM, Chao Fan wrote:
>> On Fri, Oct 20, 2017 at 10:37:52AM +0800, Dou Liyang wrote:
>> > Hi Chao,
>> > 
>> Hi Dou-san,
>> 
>> > Cheer! I have some concerns below.
>> 
>> Thanks for your reply.
>> 
>> > 
>> > At 10/19/2017 06:02 PM, Chao Fan wrote:
>> > > Here is a problem:
>> > > Here is a machine with several NUMA nodes and some of them are hot-pluggable.
>> > > It's not good for kernel to be extracted in the memory region of movable node.
>> > > But in current code, I print the address choosen by kaslr and found it may be
>> > > placed in movable node sometimes. To solve this problem, it's better to limit
>> > > the memory region choosen by kaslr to immovable node in kaslr.c. But the memory
>> > > infomation about if it's hot-pluggable is stored in ACPI SRAT table, which is
>> > > parsed after kernel is extracted. So we can't get the detail memory infomation
>> > > before extracting kernel.
>> > > 
>> > > So extend the movable_node to movable_node=nn@ss, in which nn means
>> > > the size of memory in *immovable* node, and ss means the start position of
>> > > this memory region. Then limit kaslr choose memory in these regions.
>> > 
>> > Yes, great. Here we should remember that the situation of
>> > 'movable_node=nn@ss' is rare, normal situation is 'movable_node=nn'.
>> > 
>> > So, we should consider our code tendencies for normal situation. ;-)
>> 
>> Yes, it's normal. But you can not make sure the special situation will
>> never happen,. If it happens, we can make sure codes work well, right?
>> 
>> We can not make sure that the movable nodes are continuous, or even if
>> the movable nodes are continuous, we can not make sure the memory
>> address are continuous.
>> 
>> It is easy to avoid the memory region in movable node.
>> But if we can handle more special situations, and at the same time,
>> make kernel more safe, why not?
>
>You misunderstand my opinions, I means that
>when we code, we need to know the problem clearly and which part of
>problem will often be executed.
>
>Make our code more suitable for the normal situation without affecting the
>function of the problem.
>Just like:
>
>likely() and unlikely()
Hi Dou-san,

Thanks for that. I think likely() is suitable for another place.

Thanks,
Chao Fan

>
>Here I guess you don't consider that. so I said that.
>
>> 
>> > 
>> > > 
>> > > There are two policies:
>> > > 1. Specify the memory region in *movable* node to avoid:
>> > >    Then we can use the existing mem_avoid to handle. But if the memory
>> > >    one movable node was separated by memory hole or different movable nodes
>> > >    are discontinuous, we don't know how many regions need to avoid.
>> > 
>> > It is not a problem.
>> > 
>> > As you said, we should provide an interface for users later, like that:
>> > 
>> > # cat /sys/device/system/memory/movable_node
>> > nn@ss
>> > 
>> 
>> Both are OK. I think outputing the memory region in movable_node or
>> immovable_node are both reasonable. So the interface of both methods
>> will be useful. And after we decided which policy used in kaslr, then
>> add the interface of /sys.
>> 
>
>Actually, I prefer the first one, are you ready to post the patches
>for the first policy?
>
>Thanks,
>	dou.
>> Thanks,
>> Chao Fan
>> 
>> > 
>> > Thanks,
>> > 	dou.
>> > >    OTOH, we must avoid all of the movable memory, otherwise, kaslr may
>> > >    choose the wrong place.
>> > > 2. Specify the memory region in "immovable* node to select:
>> > >    Only support 4 regions in this parameter. Then user can use two nodes
>> > >    at least for kaslr to choose, it's enough for the kernel to extract.
>> > >    At the same time, because we need only 4 new mem_vector, the usage
>> > >    of memory here is not too big.
>> > > 
>> > > PATCH 1/4 parse the extended movable_node=nn[KMG]@ss[KMG], then
>> > > 	  store the memory regions.
>> > > PATCH 2/4 selects the memory region in immovable node when process
>> > > 	  memmap.
>> > > PATCH 3/4 is the change of document.
>> > > PATCH 4/4 cleans up some little problems.
>> > > 
>> > > Chao Fan (4):
>> > >   kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]
>> > >   kaslr: select the memory region in immovable node to process
>> > >   document: change the document for the extended movable_node
>> > >   kaslr: clean up a useless variable and some usless space
>> > > 
>> > >  Documentation/admin-guide/kernel-parameters.txt |   9 ++
>> > >  arch/x86/boot/compressed/kaslr.c                | 140 +++++++++++++++++++++---
>> > >  2 files changed, 131 insertions(+), 18 deletions(-)
>> > > 
>> 

Re: [PATCH 4/4] kaslr: clean up a useless variable and some usless space

[Chao Fan]

On Fri, Oct 20, 2017 at 11:19:48AM +0800, Dou Liyang wrote:
>Hi Chao,
>
>At 10/19/2017 06:02 PM, Chao Fan wrote:
>> There are two same variable "rc" in this function. One is in the
>> circulation, the other is out of the circulation. The one out will never
>> be used, so drop it.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 22330cbe8515..8a33ed82fd0b 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -198,7 +198,6 @@ static int parse_immovable_mem(char *p,
>>  static void mem_avoid_memmap(char *str)
>>  {
>>  	static int i;
>> -	int rc;
>> 
>>  	if (i >= MAX_MEMMAP_REGIONS)
>>  		return;

Hi Dou-san,

>
>Seems it is redundant too,

Thanks for your suggestion.

Thanks,
Chao Fan

>
>Thanks,
>	dou.
>
>> @@ -277,7 +276,7 @@ static int handle_mem_memmap(void)
>>  		return 0;
>> 
>>  	tmp_cmdline = malloc(len + 1);
>> -	if (!tmp_cmdline )
>> +	if (!tmp_cmdline)
>>  		error("Failed to allocate space for tmp_cmdline");
>> 
>>  	memcpy(tmp_cmdline, args, len);
>> @@ -423,7 +422,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>  	cmd_line |= boot_params->hdr.cmd_line_ptr;
>>  	/* Calculate size of cmd_line. */
>>  	ptr = (char *)(unsigned long)cmd_line;
>> -	for (cmd_line_size = 0; ptr[cmd_line_size++]; )
>> +	for (cmd_line_size = 0; ptr[cmd_line_size++];)
>>  		;
>>  	mem_avoid[MEM_AVOID_CMDLINE].start = cmd_line;
>>  	mem_avoid[MEM_AVOID_CMDLINE].size = cmd_line_size;
>> 

Re: [PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]

[Dou Liyang]

[...]
>>>>>
>>>>> +#ifdef CONFIG_MEMORY_HOTPLUG
>>>>> +static void mem_mark_immovable(char *str)
>>>>> +{
>>>>> +	int i = 0;
>>>>> +
>>>>
>>>> you have use num_immovable_region, 'i' is useless. just remove it.
>>>
>>> Using num_immovable_region makes code too long. Using i will be
>>> clear and make sure shoter than 80 characters.
>>
>> Oh, God, that's horrific. Did you find that your code is wrong?
>>
>> num_immovable_region will be reset each time.
>
> Did you test?
>

you can try with more than one movable_node, eg:

"...movable_node=128G@128G movable_node=128G@256G..."

then, you will find the problem of you code.

Thanks,
	dou

Re: [PATCH 2/4] kaslr: select the memory region in immovable node to process

[Dou Liyang]

Hi Chao,

At 10/20/2017 11:41 AM, Chao Fan wrote:
> On Fri, Oct 20, 2017 at 11:17:26AM +0800, Dou Liyang wrote:
>> Hi Chao
>>
> Hi Dou-san,
>
>> At 10/19/2017 06:02 PM, Chao Fan wrote:
>>> Since the interrelationship between e820 or efi entries and memory
>>> region in immovable_mem is different:
>>> One memory region in one node may contain several entries of e820 or
>>> efi sometimes, and one entry of e820 or efi may contain the memory in
>>> different nodes sometimes. So select the intersection as a region to
>>> process_mem_region. It may split one node or one entry to several regions.
>>>
>>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>>> ---
>>>  arch/x86/boot/compressed/kaslr.c | 72 ++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 58 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>>> index 3c1f5204693b..22330cbe8515 100644
>>> --- a/arch/x86/boot/compressed/kaslr.c
>>> +++ b/arch/x86/boot/compressed/kaslr.c
>>> @@ -563,6 +563,7 @@ static void process_mem_region(struct mem_vector *entry,
>>>  	end = min(entry->size + entry->start, mem_limit);
>>>  	if (entry->start >= end)
>>>  		return;
>>> +
>>>  	cur_entry.start = entry->start;
>>>  	cur_entry.size = end - entry->start;
>>
>> Above code has nothing to do with this patch. remove it.
>>
>>>
>>> @@ -621,6 +622,52 @@ static void process_mem_region(struct mem_vector *entry,
>>>  	}
>>>  }
>>>
>>> +static bool select_immovable_node(unsigned long long start,
>>> +				  unsigned long long size,
>>> +				  unsigned long long minimum,
>>> +				  unsigned long long image_size)
>>> +{
>>> +	struct mem_vector region;
>>> +	int i;
>>> +
>>> +	if (num_immovable_region == 0) {
>>
>> Seems it more better:
>>
>> #ifdef CONFIG_MEMORY_HOTPLUG
>> for (i = 0; i < num_immovable_region; i++) {
>>  ...
>> }
>> #else
>> ...
>> process_mem_region(&region, minimum, image_size);
>> ...
>> #endif
>
> No, if we set CONFIG_MEMORY_HOTPLUG=y(the default is y in fedora), but we do not
> use movable_node, we should go to process_mem_region straight, right?

Yes, Indeed, you are right! ;-).

> But in your change, we still go to the for(...), even if
> num_immovable_region == 0.
> So if num_immovable_region is 0, it includes the situation of
> CONFIG_MEMORY_HOTPLUG=n
>
>>
>>> +		region.start = start;
>>> +		region.size = size;
>>> +		process_mem_region(&region, minimum, image_size);
>>> +
>>> +		if (slot_area_index == MAX_SLOT_AREA) {
>>> +			debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>>> +			return 1;
>>> +		}
>>> +	} else {
>>> +		for (i = 0; i < num_immovable_region; i++) {
>>> +			unsigned long long end, select_end;
>>> +			unsigned long long region_start, region_end;
>>> +
>>> +			end = start + size - 1;
>>> +			region_start = immovable_mem[i].start;
>>> +			region_end = region_start + immovable_mem[i].size - 1;
>>> +
>>> +			if (end < region_start || start > region_end)
>>> +				continue;
>>> +
>>> +			region.start = start > region_start ?
>>> +				       start : region_start;
>>> +			select_end = end > region_end ? region_end : end;
>>> +
>>> +			region.size = select_end - region.start + 1;
>>> +
>>> +			process_mem_region(&region, minimum, image_size);
>>> +
>>> +			if (slot_area_index == MAX_SLOT_AREA) {
>>> +				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>>> +				return 1;
>>> +			}
>>> +		}
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>>  #ifdef CONFIG_EFI
>>>  /*
>>>   * Returns true if mirror region found (and must have been processed
>>> @@ -631,7 +678,6 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>>>  {
>>>  	struct efi_info *e = &boot_params->efi_info;
>>>  	bool efi_mirror_found = false;
>>> -	struct mem_vector region;
>>>  	efi_memory_desc_t *md;
>>>  	unsigned long pmap;
>>>  	char *signature;
>>> @@ -664,6 +710,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>>>  	}
>>>
>>>  	for (i = 0; i < nr_desc; i++) {
>>> +		unsigned long long start, size;
>>> +
>>>  		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
>>>
>>>  		/*
>>> @@ -684,13 +732,11 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>>>  		    !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
>>>  			continue;
>>>
>>> -		region.start = md->phys_addr;
>>> -		region.size = md->num_pages << EFI_PAGE_SHIFT;
>>> -		process_mem_region(&region, minimum, image_size);
>>> -		if (slot_area_index == MAX_SLOT_AREA) {
>>> -			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
>>> +		start = md->phys_addr;
>>> +		size = md->num_pages << EFI_PAGE_SHIFT;
>>> +
>>> +		if (select_immovable_node(start, size, minimum, image_size))
>>
>> Why you replace the region with two parameters? Just use region.
>>
>
> You can see the commit message, we may splite one entry to more regions,
> so there will be region.start compared with memory region in
> immovable_mem and then fill more new regions if we use region here.
> The code will look strange.
>
> Start and size will be more clear.

OK, Understood!

Thanks,
	dou.

Re: [PATCH 0/4] kaslr: extend movable_node to movable_node=nn[KMG]@ss[KMG]

[Dou Liyang]

Hi Chao,

[...]
>
> Thanks for that. I think likely() is suitable for another place.

Aha, Just as an example, don't need to use it. ;-)

>>>
>>> Both are OK. I think outputing the memory region in movable_node or
>>> immovable_node are both reasonable. So the interface of both methods
>>> will be useful. And after we decided which policy used in kaslr, then
>>> add the interface of /sys.

Yes, so great.

>>>
>>
>> Actually, I prefer the first one, are you ready to post the patches
>> for the first policy?
>>

I guess you may miss this, I want to know if you have already do the
first policy. then we can compare and decide which one is better. If
not, I can do that! let's make KASLR work well with node hot-plug
quickly.

cc Baoquan,

do you have any ideas, ;-)

Thanks,
	dou.

Re: [PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]

[Chao Fan]

On Fri, Oct 20, 2017 at 01:33:16PM +0800, Dou Liyang wrote:
>[...]
>> > > > > 
>> > > > > +#ifdef CONFIG_MEMORY_HOTPLUG
>> > > > > +static void mem_mark_immovable(char *str)
>> > > > > +{
>> > > > > +	int i = 0;
>> > > > > +
>> > > > 
>> > > > you have use num_immovable_region, 'i' is useless. just remove it.
>> > > 
>> > > Using num_immovable_region makes code too long. Using i will be
>> > > clear and make sure shoter than 80 characters.
>> > 
>> > Oh, God, that's horrific. Did you find that your code is wrong?
>> > 
>> > num_immovable_region will be reset each time.
>> 
>> Did you test?
>> 
>
>you can try with more than one movable_node, eg:
>
>"...movable_node=128G@128G movable_node=128G@256G..."
>

Yes, you are right.
I thought users will specify the regions like this:
movable_node=1G@2G,1G@4G
If users want to use
movable_node=1G@2G movable_node=1G@4G
Your suggestion is helpful and I will update it.

Thanks,
Chao Fan
>then, you will find the problem of you code.
>
>Thanks,
>	dou