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

Chao Fan
2017-10-20T03:22:31Z

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;
>>
>>