Date: Thu, 7 Dec 2017 11:09:24 +0800
From: Baoquan He <bhe@redhat.com>
To: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, hpa@zytor.com,
        tglx@linutronix.de, mingo@redhat.com, keescook@chromium.org,
        yasu.isimatu@gmail.com, indou.takao@jp.fujitsu.com,
        caoj.fnst@cn.fujitsu.com, douly.fnst@cn.fujitsu.com
Subject: Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to
 specify extracting memory
Message-ID: <20171207030924.GO15074@x1>
References: <20171205085200.9528-1-fanc.fnst@cn.fujitsu.com>
 <20171205085200.9528-2-fanc.fnst@cn.fujitsu.com>
 <20171206093557.GL15074@x1>
 <20171207025356.GD28884@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171207025356.GD28884@localhost.localdomain>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6832
Lines: 205

On 12/07/17 at 10:53am, Chao Fan wrote:
> On Wed, Dec 06, 2017 at 05:35:57PM +0800, Baoquan He wrote:
> >Hi Chao,
> >
> >Yes, now the code looks much better than the last version.
> >
> >On 12/05/17 at 04:51pm, Chao Fan wrote:
> >> In current code, kaslr may choose the memory region in movable
> >> nodes to extract kernel, which will make the nodes can't be hot-removed.
> >> To solve it, we can specify the memory region in immovable node.
> >> Create immovable_mem to store the regions in immovable_mem, where should
> >> be chosen by kaslr.
> >> 
> >> Multiple regions can be specified, comma delimited.
> >> Considering the usage of memory, only support for 4 regions.
> >> 4 regions contains 2 nodes at least, enough for kernel to extract.
> >> 
> >> Also change the "handle_mem_memmap" to "handle_mem_filter", since
> >> it will not only handle memmap parameter now.
> >
> >One concern is whether it will fail to do KASLR if only specify
> 
> Sorry, I think I have not understood your point.
> So if there is something wrong, please let me know.

What I meant is whether we need check 'movable_node' and 
'immovable_mem=' being specified together. If only specify 'movable_node',
we may need to return and do not do kaslr or do not do physical kaslr
since kernel could be located on movable mem region.

Otherwise it will do physical kaslr anyway, memory hotplug will be
impacted later.

> 
> I don't think if only specify "movable_node" will fail KASLR.
> Since in this patchset(3/4), only disable kernel mirror. KASLR in
> current upstream code didn't parse "movable_node".
> 
> >"movable_node". Surely in this case it won't fail system, just hotplug
> >memory might be impacted if kernel is located on that, will FJ mind
> 
> Yes, it's the reason why I make this patchset.
> In my personal understanding, "movable_node" is a beginning why I make
> this patchset, but not the whole reason.
> Only "movable_node" specified might cause hotplug memory can't be
> removed if kernel is located on that, so we need the help of
> "immovable_mem=". "movable_node" help hotplug memory can be removed, and
> "immovable_mem=" works for the same target, but just in kaslr.
> So up to now, there is not a very tight coupling between "movable_node"
> and "immovable_mem=". The independence of "immovable_mem=" is that,
> help kaslr selects the right regions, avoid the memory in hotpluggable
> NUMA nodes, which causes the memory can't removed. It's a independent
> reason why we need a parameter like "immovable_mem=".
> So I think we should also handle it if only specify "immovable_mem="
> without "movable_node".
> 
> Thanks,
> Chao Fan
> 
> >this? And what if only specify 'immovable_mem=' but without 'movable_node'?
> >
> >Thanks
> >Baoquan
> >
> >> 
> >> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> >> ---
> >>  arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 77 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> >> index a63fbc25ce84..0bbbaf5f6370 100644
> >> --- a/arch/x86/boot/compressed/kaslr.c
> >> +++ b/arch/x86/boot/compressed/kaslr.c
> >> @@ -108,6 +108,15 @@ 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
> >> +
> >> +/* Store the memory regions in immovable node */
> >> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
> >> +
> >> +/* The immovable regions user specify, not more than 4 */
> >> +static int num_immovable_region;
> >> +
> >>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
> >>  {
> >>  	/* Item one is entirely before item two. */
> >> @@ -168,6 +177,38 @@ 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;
> >> +
> >> +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
> >> +	switch (*p) {
> >> +	case '@':
> >> +		*start = memparse(p + 1, &p);
> >> +		return 0;
> >> +	default:
> >> +		/*
> >> +		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
> >> +		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
> >> +		 * the region starts from 0.
> >> +		 */
> >> +		*start = 0;
> >> +		return 0;
> >> +	}
> >> +
> >> +	return -EINVAL;
> >> +}
> >> +
> >>  static void mem_avoid_memmap(char *str)
> >>  {
> >>  	static int i;
> >> @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
> >>  		memmap_too_large = true;
> >>  }
> >>  
> >> -static int handle_mem_memmap(void)
> >> +#ifdef CONFIG_MEMORY_HOTPLUG
> >> +static void parse_immovable_mem_regions(char *str)
> >> +{
> >> +	static int i;
> >> +
> >> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
> >> +		int rc;
> >> +		unsigned long long start, size;
> >> +		char *k = strchr(str, ',');
> >> +
> >> +		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++;
> >> +	}
> >> +	num_immovable_region = i;
> >> +}
> >> +#else
> >> +static inline void parse_immovable_mem_regions(char *str)
> >> +{
> >> +}
> >> +#endif
> >> +
> >> +static int handle_mem_filter(void)
> >>  {
> >>  	char *args = (char *)get_cmd_line_ptr();
> >>  	size_t len = strlen((char *)args);
> >> @@ -215,7 +286,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, "immovable_mem="))
> >>  		return 0;
> >>  
> >>  	tmp_cmdline = malloc(len + 1);
> >> @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
> >>  
> >>  		if (!strcmp(param, "memmap")) {
> >>  			mem_avoid_memmap(val);
> >> +		} else if (!strcmp(param, "immovable_mem")) {
> >> +			parse_immovable_mem_regions(val);
> >>  		} else if (!strcmp(param, "mem")) {
> >>  			char *p = val;
> >>  
> >> @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> >>  	/* We don't need to set a mapping for setup_data. */
> >>  
> >>  	/* Mark the memmap regions we need to avoid */
> >> -	handle_mem_memmap();
> >> +	handle_mem_filter();
> >>  
> >>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
> >>  	/* Make sure video RAM can be used. */
> >> -- 
> >> 2.14.3
> >> 
> >> 
> >> 
> >
> >
> 
>