Re: [PATCH v5 4/4] x86/boot/KASLR: Limit kaslr to choosing the immovable memory

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

On Mon, Aug 27, 2018 at 01:56:07PM +0800, Baoquan He wrote:
>On 08/07/18 at 02:50pm, Chao Fan wrote:
>> If 'CONFIG_MEMORY_HOTREMOVE' specified and the account of immovable
>If CONFIG_MEMORY_HOTREMOVE is enabled, 
>> memory regions is not zero. Calculate the intersection between memory
>> regions from e820/efi memory table and immovable memory regions.
>> Or go on the old code.
>> 
>> Rename process_mem_region to slots_count to match slots_fetch_random,
>> and name new function as process_mem_region.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 66 ++++++++++++++++++++++++++------
>>  1 file changed, 55 insertions(+), 11 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 720878f967a3..9c6e24a23a2d 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -635,9 +635,9 @@ static unsigned long slots_fetch_random(void)
>>  	return 0;
>>  }
>>  
>> -static void process_mem_region(struct mem_vector *entry,
>> -			       unsigned long minimum,
>> -			       unsigned long image_size)
>> +static void slots_count(struct mem_vector *entry,
>> +			unsigned long minimum,
>> +			unsigned long image_size)
>>  {
>>  	struct mem_vector region, overlap;
>>  	struct slot_area slot_area;
>> @@ -714,6 +714,56 @@ static void process_mem_region(struct mem_vector *entry,
>>  	}
>>  }
>>  
>> +static bool process_mem_region(struct mem_vector *region,
>> +			       unsigned long long minimum,
>> +			       unsigned long long image_size)
>> +{
>
>Is it possible to take num_immovable_mem definition out from #ifdef
>CONFIG_MEMORY_HOTREMOVE block and check it here like below? This way,
>one level of indentation can be reduced in the for loop, and code is
>more readable.
>

I think there is a mistake.

The logical is:
if (#ifdef CONFIG_MEMORY_HOTREMOVE) && (num_immovable_mem > 0)
	then A;
else
	then B;

But below is:
if (num_immovable_mem > 0)
	then B;
else if (#ifdef CONFIG_MEMORY_HOTREMOVE)
	then A;
else
	nothing;

The precondition of the loop is (num_immovable_mem > 0), because
there is only one condition that we need go the A code:
CONFIG_MEMORY_HOTREMOVE is defined, and memory information in srat
found.

But there is many conditions we go the B code:
1. CONFIG_MEMORY_HOTREMOVE is not defined.
2. CONFIG_MEMORY_HOTREMOVE defined, but we didn't get the right acpi tables
3. CONFIG_MEMORY_HOTREMOVE defined, or there is only one node in this machine.

Yes, the code is hard to read, but you have changed the logical, there
is a compromise method, I don't know whether is better:

#ifdef CONFIG_MEMORY_HOTREMOVE
	if (num_immovable_mem == 0)
		goto B;

	for (i = 0; i < num_immovable_mem; i++) {
		...
	}
#endif

B:
	slots_count(region, minimum, image_size);
	
	if (slot_area_index == MAX_SLOT_AREA) {
		debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
		return 1;
	}
	return 0;
	

>
>static bool process_mem_region(struct mem_vector *region,
>			       unsigned long long minimum,
>			       unsigned long long image_size)
>{
>
>	/*
>	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
>	 * walk all the regions, so use region directely.
>	 */
>	if (num_immovable_mem > 0) {
>		slots_count(region, minimum, image_size);
>		
>		if (slot_area_index == MAX_SLOT_AREA) {
>			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
>			return 1;
>		}
>		return 0;
>	}
>
>#ifdef CONFIG_MEMORY_HOTREMOVE
>	for (i = 0; i < num_immovable_mem; i++) {
>		...
>	}
>#endif
>}
>