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

[Alistair Popple <apopple@nvidia.com>]

Thomas Gleixner <tglx@linutronix.de> writes:

> 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 :)

It's a much more plesent 7C in the upside down half of the world atm :)

Anyway I will put together a proper patch to do the above.