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