Re: [PATCH v4] x86: fix kaslr and memmap collision

[Baoquan He]

On 01/04/17 at 10:06am, Dave Jiang wrote:
> On 01/03/2017 07:37 PM, Baoquan He wrote:
> >>  #include <generated/compile.h>
> >>  #include <linux/module.h>
> >> @@ -61,9 +62,16 @@ enum mem_avoid_index {
> >>  	MEM_AVOID_INITRD,
> >>  	MEM_AVOID_CMDLINE,
> >>  	MEM_AVOID_BOOTPARAMS,
> >> +	MEM_AVOID_MEMMAP1,
> >> +	MEM_AVOID_MEMMAP2,
> >> +	MEM_AVOID_MEMMAP3,
> >> +	MEM_AVOID_MEMMAP4,
> > 
> > This looks not good. Could it be done like fixed_addresses?
> > Something like:
> > 
> > 	MEM_AVOID_MEMMAP_BEGIN,
> > 	MEM_AVOID_MEMMAP_BEGIN + MEM_AVOID_MAX -1,
> > 
> > Please point it out to me if there's some existing code in kernel like
> > your way, I can also accept it.
> 
> I think you mean:
> MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1

Ah, yes. Sorry for this.

> 
> I will change
> 
> 
> > 
> >>  	MEM_AVOID_MAX,
> >>  };
> >>  
> >> +/* only supporting at most 4 memmap regions with kaslr */
> > And here, "Only supporting at most 4 un-usable memmap regions with kaslr"?
> > Surely this is based on if you will ignore the usable memory and do not
> > store it as 0. And also the log need be changed too accordingly.
> >> +#define MAX_MEMMAP_REGIONS	4
...
> >> +static int
> >> +parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> >> +{
> >> +	char *oldp;
> >> +
> >> +	if (!p)
> >> +		return -EINVAL;
> >> +
> >> +	/* we don't care about this option here */
> >> +	if (!strncmp(p, "exactmap", 8))
> >> +		return -EINVAL;
> >> +
> >> +	oldp = p;
> >> +	*size = _memparse(p, &p);
> >> +	if (p == oldp)
> >> +		return -EINVAL;
> >> +
> >> +	switch (*p) {
> >> +	case '@':
> >> +		/* skip this region, usable */
> >> +		*start = 0;
> >> +		*size = 0;
> >> +		return 0;
> > How about direclty return if nn@ss? Seems no need to waste one mem avoid
> > region slot. In fact even amount of usable memory regions are provided to
> > 100, it won't impact that you want to specify a reserve memmap region if
> > you skip it direclty. Personal opinion.
> 
> We are not wasting the slot. If you look mem_avoid_memmap() where I call
> the function, it will skip with a continue if size == 0 without
> incrementing the 'i' counter. That will skip all the nn@ss regions
> without counting against the max avoid mapping.

Yes, indeed. Sorry, I didn't read the code carefully.

Thanks
Baoquan