Re: [PATCHv3] x86/boot/KASLR: skip the specified crashkernel region

[Chao Fan]

On Tue, Apr 02, 2019 at 12:10:46PM +0800, Pingfan Liu wrote:
>crashkernel=x@y or or =range1:size1[,range2:size2,...]@offset option may
or or?
>fail to reserve the required memory region if KASLR puts kernel into the
>region. To avoid this uncertainty, asking KASLR to skip the required
>region.
>
>Signed-off-by: Pingfan Liu <kernelfans@xxxxxxxxx>
>Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>Cc: Borislav Petkov <bp@xxxxxxxxx>
>Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>Cc: Baoquan He <bhe@xxxxxxxxxx>
>Cc: Will Deacon <will.deacon@xxxxxxx>
>Cc: Nicolas Pitre <nico@xxxxxxxxxx>
>Cc: Pingfan Liu <kernelfans@xxxxxxxxx>
>Cc: Chao Fan <fanc.fnst@xxxxxxxxxxxxxx>
>Cc: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
>Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>Cc: linux-kernel@xxxxxxxxxxxxxxx
>---
[...]
>+
>+/* handle crashkernel=x@y or =range1:size1[,range2:size2,...]@offset options */

Before review, I want to say more about the background.
It's very hard to review the code for someone who is not so familiar
with kdump, so could you please explain more ahout
the uasge of crashkernel=range1:size1[,range2:size2,...]@offset.
And also there are so many jobs who are parsing string. So I really
need your help to understand the PATCH.

>+static void mem_avoid_specified_crashkernel_region(char *option)
>+{
>+	unsigned long long crash_size, crash_base = 0;
>+	char	*first_colon, *first_space, *cur = option;
Is there a tab after char?
>+
>+	first_colon = strchr(option, ':');
>+	first_space = strchr(option, ' ');
>+	/* if contain ":" */
>+	if (first_colon && (!first_space || first_colon < first_space)) {
>+		int i;
>+		u64 total_sz = 0;
>+		struct boot_e820_entry *entry;
>+
>+		for (i = 0; i < boot_params->e820_entries; i++) {
>+			entry = &boot_params->e820_table[i];
>+			/* Skip non-RAM entries. */
>+			if (entry->type != E820_TYPE_RAM)
>+				continue;
>+			total_sz += entry->size;
I wonder whether it's needed to consider the memory ranges here.
I think it's OK to only record the regions should to be avoid.
I remeber I ever talked with Baoquan about the similiar problems.
@Baoquan, I am not sure if I misunderstand something.

Thanks,
Chao Fan
>+		}
>+		handle_crashkernel_mem(option, total_sz, &crash_size,
>+			&crash_base);
>+	} else {
>+		crash_size = memparse(option, &cur);
>+		if (option == cur)
>+			return;
>+		while (*cur && *cur != ' ' && *cur != '@')
>+			cur++;
>+		if (*cur == '@') {
>+			option = cur + 1;
>+			crash_base = memparse(option, &cur);
>+		}
>+	}
>+	if (crash_base) {
>+		mem_avoid[MEM_AVOID_CRASHKERNEL].start = crash_base;
>+		mem_avoid[MEM_AVOID_CRASHKERNEL].size = crash_size;
>+	} else {
>+		/*
>+ 		 * Clearing mem_avoid if no offset is given. This is consistent
>+ 		 * with kernel, which uses the last crashkernel= option.
>+		 */
>+		mem_avoid[MEM_AVOID_CRASHKERNEL].start = 0;
>+		mem_avoid[MEM_AVOID_CRASHKERNEL].size = 0;
>+	}
>+}
> 
> static void handle_mem_options(void)
> {
>@@ -248,7 +358,7 @@ static void handle_mem_options(void)
> 	u64 mem_size;
> 
> 	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>-		!strstr(args, "hugepages"))
>+		!strstr(args, "hugepages") && !strstr(args, "crashkernel="))
> 		return;
> 
> 	tmp_cmdline = malloc(len + 1);
>@@ -284,6 +394,8 @@ static void handle_mem_options(void)
> 				goto out;
> 
> 			mem_limit = mem_size;
>+		} else if (strstr(param, "crashkernel")) {
>+			mem_avoid_specified_crashkernel_region(val);
> 		}
> 	}
> 
>@@ -412,7 +524,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 */
>+	/* Mark the regions we need to avoid */
> 	handle_mem_options();
> 
> 	/* Enumerate the immovable memory regions */
>-- 
>2.7.4
>
>
>