Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Aug 12 2024 at 17:41, Alistair Popple wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>> ---
>> --- a/arch/x86/include/asm/pgtable_64_types.h
>> +++ b/arch/x86/include/asm/pgtable_64_types.h
>> @@ -187,6 +187,8 @@ extern unsigned int ptrs_per_p4d;
>>  #define KMSAN_MODULES_ORIGIN_START	(KMSAN_MODULES_SHADOW_START + MODULES_LEN)
>>  #endif /* CONFIG_KMSAN */
>>  
>> +#define DIRECT_MAP_END		(VMALLOC_START - 1)
>
> If I'm understanding the KASLR implementation correctly then this
> doesn't seem quite right - the entropy means there is a hole from the
> end of the direct map (ie. the range PAGE_OFFSET to
> PAGE_OFFSET+kaslr_regions[0].size_tb) and VMALLOC_START which shouldn't
> be used.
>
> In practice hotplugging memory into that range probably works, but it
> seems like it would set us up for future bugs. It also means memory
> hotplug could fail intermittently based on the per-boot entropy.
>
> For example with CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING set to 10T one
> could imagine hotplugging 16T would mostly work except on some system
> boots if KASLR happens to randomly place VMALLOC_START close to the end
> of the direct map.
>
> Therefore to keep memory hotplug deterministic I think it would be
> better to define DIRECT_MAP_END as a variable that KASLR sets/updates.

Right. I forgot about the hole.

> This does not look right to me - isn't DIRECT_MAP_END a virtual address
> where as the resource ranges are physical addresses? Ie. I think this
> should be:
>
> +		end = min_t(resource_size_t, base->end, __pa(DIRECT_MAP_END));
>
> The same applies to the rest of the DIRECT_MAP_END users here. Perhaps
> it would be better to define this as DIRECT_MAP_SIZE and calculate this
> based off PAGE_OFFSET instead?

Duh, yes. I shouldn't try to write patches at 30C :)