Messages in this thread | | | Date | Fri, 20 Oct 2017 11:22:03 +0800 | From | Chao Fan <> | Subject | Re: [PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG] |
| |
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; >> >>
| |