Messages in this thread | | | Date | Fri, 20 Oct 2017 11:44:21 +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: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; >> > > >> > > >>
| |