Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754859AbdLFKDL (ORCPT ); Wed, 6 Dec 2017 05:03:11 -0500 Received: from mail.cn.fujitsu.com ([183.91.158.132]:56086 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754703AbdLFKDG (ORCPT ); Wed, 6 Dec 2017 05:03:06 -0500 X-IronPort-AV: E=Sophos;i="5.43,368,1503331200"; d="scan'208";a="30760666" Date: Wed, 6 Dec 2017 18:02:38 +0800 From: Chao Fan To: Baoquan He CC: Kees Cook , LKML , X86 ML , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , , , , Dou Liyang Subject: Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node Message-ID: <20171206100238.GB28884@localhost.localdomain> References: <20171205085200.9528-1-fanc.fnst@cn.fujitsu.com> <20171205085200.9528-3-fanc.fnst@cn.fujitsu.com> <20171206092800.GK15074@x1> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20171206092800.GK15074@x1> User-Agent: Mutt/1.9.1 (2017-09-22) X-Originating-IP: [10.167.225.56] X-yoursite-MailScanner-ID: 57FB0486A768.ABC80 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: fanc.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6210 Lines: 180 On Wed, Dec 06, 2017 at 05:28:00PM +0800, Baoquan He wrote: >On 12/05/17 at 11:40am, Kees Cook wrote: >> On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan wrote: >> > If there is no immovable memory region specified, go on the old code. >> > There are several conditons: >> > 1. CONFIG_MEMORY_HOTPLUG is not specified to y. >> > 2. immovable_mem= is not specified. >> > >> > Otherwise, calculate the intersecting between memmap entry and >> > immovable memory. >> >> Instead of copy/pasting code between process_efi_entries() and >> process_e820_entries(), I'd rather that process_mem_region() is >> modified to deal with immovable regions. > >If put it into process_mem_region(), one level of loop is added. How Yes, one new loop will add ahead of the while() in process_mem_region then the code may look like: @@ -509,6 +555,24 @@ static void process_mem_region(struct mem_vector *entry, region.start = cur_entry.start; region.size = cur_entry.size; +#ifdef CONFIG_MEMORY_HOTPLUG +next: + if (num_immovable_mem > 0) { + unsigned long long start, reg_end; + + if (!mem_overlaps(&entry, &immovable_mem[i])) + goto out; + + start = immovable_mem[i].start; + end = start + immovable_mem[i].size; + + region.start = clamp(cur_entry.start, start, end); + reg_end = clamp(cur_entry.start + cur_entry.size, start, end); + + region.size = region_end - region.start; + } +#endif + /* Give up if slot area array is full. */ while (slot_area_index < MAX_SLOT_AREA) { start_orig = region.start; @@ -522,7 +586,7 @@ static void process_mem_region(struct mem_vector *entry, /* Did we raise the address above the passed in memory entry? */ if (region.start > cur_entry.start + cur_entry.size) - return; + goto out; /* Reduce size by any delta from the original address. */ region.size -= region.start - start_orig; @@ -534,12 +598,12 @@ static void process_mem_region(struct mem_vector *entry, /* Return if region can't contain decompressed kernel */ if (region.size < image_size) - return; + goto out; /* If nothing overlaps, store the region and return. */ if (!mem_avoid_overlap(®ion, &overlap)) { store_slot_info(®ion, image_size); - return; + goto out; } /* Store beginning of region if holds at least image_size. */ @@ -553,12 +617,20 @@ static void process_mem_region(struct mem_vector *entry, /* Return if overlap extends to or past end of region. */ if (overlap.start + overlap.size >= region.start + region.size) - return; + goto out; /* Clip off the overlapping region and start over. */ region.size -= overlap.start - region.start + overlap.size; region.start = overlap.start + overlap.size; } + +out: +#ifdef CONFIG_MEMORY_HOTPLUG + i++; + if (i < num_immovable_mem) + goto next; +#endif + return; } >about changing it like below. If no immovable_mem, just process the >region in process_immovable_mem(). This we don't need to touch >process_mem_region(). Yes, Baoquan's method will make all change be in one function. Kees, how do you think, which is better? Thanks, Chao Fan > > >From 9ae3f5ab0e2f129757495af2412bd52dcf86aa46 Mon Sep 17 00:00:00 2001 >From: Baoquan He >Date: Wed, 6 Dec 2017 17:24:55 +0800 >Subject: [PATCH] KASLR: change code > >Signed-off-by: Baoquan He >--- > arch/x86/boot/compressed/kaslr.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > >diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c >index 13d26b859c69..73b1562a7439 100644 >--- a/arch/x86/boot/compressed/kaslr.c >+++ b/arch/x86/boot/compressed/kaslr.c >@@ -638,13 +638,23 @@ static bool process_immovable_mem(struct mem_vector region, > unsigned long long minimum, > unsigned long long image_size) > { >- int i; >+ /* If no immovable_mem stored, use region directly */ >+ if (num_immovable_region == 0) { >+ process_mem_region(&entry, minimum, image_size); >+ >+ if (slot_area_index == MAX_SLOT_AREA) { >+ debug_putstr("Aborted memmap scan (slot_areas full)!\n"); >+ return 1; >+ } >+ >+ return 0; >+ } > > /* > * Walk all immovable regions, and filter the intersection > * to process_mem_region. > */ >- for (i = 0; i < num_immovable_region; i++) { >+ for (int i = 0; i < num_immovable_region; i++) { > struct mem_vector entry; > unsigned long long start, end, entry_end; > >@@ -738,14 +748,7 @@ process_efi_entries(unsigned long minimum, unsigned long image_size) > region.start = md->phys_addr; > region.size = md->num_pages << EFI_PAGE_SHIFT; > >- /* If no immovable_mem stored, use region directly */ >- if (num_immovable_region == 0) { >- process_mem_region(®ion, minimum, image_size); >- if (slot_area_index == MAX_SLOT_AREA) { >- debug_putstr("Aborted memmap scan (slot_areas full)!\n"); >- break; >- } >- } else if (process_immovable_mem(region, minimum, image_size)) >+ if (process_immovable_mem(region, minimum, image_size)) > break; > } > return true; >@@ -774,14 +777,7 @@ static void process_e820_entries(unsigned long minimum, > region.start = entry->addr; > region.size = entry->size; > >- /* If no immovable_mem stored, use region directly */ >- if (num_immovable_region == 0) { >- process_mem_region(®ion, minimum, image_size); >- if (slot_area_index == MAX_SLOT_AREA) { >- debug_putstr("Aborted memmap scan (slot_areas full)!\n"); >- break; >- } >- } else if (process_immovable_mem(region, minimum, image_size)) >+ if (process_immovable_mem(region, minimum, image_size)) > break; > } > } >-- >2.5.5 > > >