Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node

[Chao Fan <fanc.fnst@cn.fujitsu.com>]

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 <fanc.fnst@cn.fujitsu.com> 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(&region, &overlap)) {
                        store_slot_info(&region, 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 <bhe@redhat.com>
>Date: Wed, 6 Dec 2017 17:24:55 +0800
>Subject: [PATCH] KASLR: change code
>
>Signed-off-by: Baoquan He <bhe@redhat.com>
>---
> 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(&region, 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(&region, 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
>
>
>