Re: [PATCH] x86/mm: Handle alloc failure in phys_*_init()

["H. Peter Anvin" <hpa@zytor.com>]

On June 4, 2025 11:36:45 PM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Em Sharnoff <sharnoff@neon.tech> wrote:
>
>> tl;dr:
>> 
>> * When setting up page table mappings for physical addresses after boot,
>>   alloc_low_page() uses GFP_ATOMIC, which is allowed to fail.
>> * This isn't currently handled, and results in a null pointer
>>   dereference when it occurs.
>> * This allocation failure can happen during memory hotplug.
>> 
>> To handle failure, change phys_pud_init() and similar functions to
>> return zero if allocation failed (either directly or transitively), and
>> convert that to -ENOMEM in arch_add_memory().
>
>> +		/*
>> +		 * Bail only after updating pgd/p4d to keep progress from p4d across retries.
>> +		 */
>> +		if (!paddr_last)
>> +			return 0;
>> +
>>  		pgd_changed = true;
>
>> -	init_memory_mapping(start, start + size, params->pgprot);
>> +	if (!init_memory_mapping(start, start + size, params->pgprot))
>> +		return -ENOMEM;
>
>I agree that it makes total sense to fix all this (especially since you 
>are actively triggering it), but have you tried also changing it away 
>from GFP_ATOMIC? There's no real reason why it should be GFP_ATOMIC 
>AFAICS, other than some historic inertia that nobody bothered to fix.
>
>Plus, could you please change the return flow from this zero 
>special-case over to something like ERR_PTR(-ENOMEM) and IS_ERR()?
>
>*Technically* zero is a valid physical address, although we 
>intentionally never use it in the kernel AFAIK and wouldn't ever put a 
>page table there either. ERR_PTR()/IS_ERR() is much easier on the eyes 
>than the zero special-case.
>
>Finally, could you make this a 2-patch fix series: first one to fix the 
>error return path to not crash, and the second one to change it away 
>from GFP_ATOMIC?
>
>Thanks,
>
>	Ingo

Specifically, zero is a valid *user space* address.