Re: [PATCH v2] x86/unaccepted_memory: Do not accept ranges that are no longer E820 RAM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] x86/unaccepted_memory: uncleared hole bits in
  arch_clear_accepted()
- [Medium] x86/unaccepted_memory: outward unit rounding in
  arch_clear_accepted()
--

commit ddcd20a91b6aa10c63b863069cf031e61f95c5fc
Author: Pankaj Gupta <pankaj.gupta@amd.com>

x86/unaccepted_memory: Do not accept ranges that are no longer E820 RAM

This commit restricts x86 memory acceptance to page-aligned E820_TYPE_RAM
ranges and updates accept_memory() to clear bitmap bits via
arch_clear_accepted() only for ranges that were accepted.

> diff --git a/arch/x86/include/asm/unaccepted_memory.h b/arch/x86/include/asm/unaccepted_memory.h
> index f5937e9866ac1..9e27a9953dd83 100644
> --- a/arch/x86/include/asm/unaccepted_memory.h
> +++ b/arch/x86/include/asm/unaccepted_memory.h
> @@ -2,10 +2,11 @@
>  #define _ASM_X86_UNACCEPTED_MEMORY_H
>  
>  #include <linux/efi.h>
> +#include <asm/e820/api.h>
>  #include <asm/tdx.h>
>  #include <asm/sev.h>
>  
> -static inline void arch_accept_memory(phys_addr_t start, phys_addr_t end)
> +static inline void __arch_accept_memory(phys_addr_t start, phys_addr_t end)
>  {
>  	/* Platform-specific memory-acceptance call goes here */
>  	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
> @@ -18,6 +19,47 @@ static inline void arch_accept_memory(phys_addr_t start, phys_addr_t end)
>  	}
>  }
>  
> +static inline bool e820_ram_pages(struct e820_entry *entry, phys_addr_t start,
> +				  phys_addr_t end, phys_addr_t *s, phys_addr_t *e)
> +{
> +	if (entry->type != E820_TYPE_RAM)
> +		return false;
> +
> +	*s = PAGE_ALIGN(max_t(phys_addr_t, start, entry->addr));
> +	*e = PAGE_ALIGN_DOWN(min_t(phys_addr_t, end, entry->addr + entry->size));
> +	return *s < *e;
> +}
> +
> +static inline void arch_accept_memory(phys_addr_t start, phys_addr_t end)
> +{
> +	phys_addr_t s, e;
> +	u32 i;
> +
> +	for (i = 0; i < e820_table->nr_entries; i++) {
> +		if (e820_ram_pages(&e820_table->entries[i], start, end, &s, &e))
> +			__arch_accept_memory(s, e);
> +	}
> +}
> +
> +static inline void arch_clear_accepted(unsigned long *bitmap, phys_addr_t base,
> +				       unsigned long unit_size,
> +				       phys_addr_t start, phys_addr_t end)
> +{
> +	phys_addr_t s, e;
> +	u32 i;
> +
> +	for (i = 0; i < e820_table->nr_entries; i++) {
> +		unsigned long first, last;
> +
> +		if (!e820_ram_pages(&e820_table->entries[i], start, end, &s, &e))
> +			continue;
> +
> +		first = (s - base) / unit_size;
> +		last = DIV_ROUND_UP(e - base, unit_size);
> +		bitmap_clear(bitmap, first, last - first);
> +	}
> +}

[Severity: Medium]
Does rounding first down and last up to unit_size boundaries clear the
bitmap bit for an entire 2 MiB unit even when only part of that unit was
accepted?

When a memmap= reservation or mem= limit is smaller than unit_size (2 MiB)
or not aligned to it, arch_accept_memory() only accepts the [s, e)
E820_TYPE_RAM sub-range within that unit and leaves the remaining pages of
the unit unaccepted in hardware.  Because first is rounded down via
(s - base) / unit_size and last is rounded up via
DIV_ROUND_UP(e - base, unit_size), arch_clear_accepted() clears the unit's
bit in unaccepted->bitmap anyway.  If a new kernel is later booted via
kexec (which reuses the EFI unaccepted memory table alongside the
pre-cmdline e820_table_kexec map), it sees the whole 2 MiB unit as
E820_TYPE_RAM with its unaccepted bit already cleared and faults when
accessing the unaccepted pages.

[ ... ]

> diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
> index 4a8ec8d6a571e..b8a0c0d1d2891 100644
> --- a/drivers/firmware/efi/unaccepted_memory.c
> +++ b/drivers/firmware/efi/unaccepted_memory.c
> @@ -130,7 +130,6 @@ void accept_memory(phys_addr_t start, unsigned long size)
>  	for_each_set_bitrange_from(range_start, range_end, unaccepted->bitmap,
>  				   range.end) {
>  		unsigned long phys_start, phys_end;
> -		unsigned long len = range_end - range_start;
>  
>  		phys_start = range_start * unit_size + unaccepted->phys_base;
>  		phys_end = range_end * unit_size + unaccepted->phys_base;
> @@ -150,7 +149,8 @@ void accept_memory(phys_addr_t start, unsigned long size)
>  		arch_accept_memory(phys_start, phys_end);
>  
>  		spin_lock(&unaccepted_memory_lock);
> -		bitmap_clear(unaccepted->bitmap, range_start, len);
> +		arch_clear_accepted(unaccepted->bitmap, unaccepted->phys_base,
> +				    unit_size, phys_start, phys_end);
>  	}

[Severity: Medium]
If a unit_size-aligned hole such as memmap=2M$0x1ffe00000 leaves its bit set
in unaccepted->bitmap, doesn't range_contains_unaccepted_memory() permanently
report the accepted RAM range immediately preceding the hole as unaccepted?

Both accept_memory() and range_contains_unaccepted_memory() extend any
unit_size-aligned end address by one extra unit_size to cover
load_unaligned_zeropad() across the boundary:

drivers/firmware/efi/unaccepted_memory.c:range_contains_unaccepted_memory() {
	...
	if (!(end % unit_size))
		end += unit_size;
	...
	while (start < end) {
		if (test_bit(start / unit_size, unaccepted->bitmap)) {
			ret = true;
			break;
		}

		start += unit_size;
	}
}

When the valid RAM before the hole is accepted, accept_memory() extends
phys_end into the hole, but arch_clear_accepted() leaves the hole's bitmap
bit set to 1 because it contains no E820_TYPE_RAM.  After that, calling
pfn_is_unaccepted_memory() on the valid, already-accepted 4 KiB RAM page
immediately below the hole (for example PFN 0x1ffdff before 0x1ffe00000)
has end aligned to unit_size, so range_contains_unaccepted_memory() steps
into the hole's unit, sees the set bit, and always returns true.  As a
result, read_kcore_iter() zero-fills that valid RAM page in /proc/kcore,
unaccepted_memory_vmcore_pfn_is_ram() omits it from /proc/vmcore, and page
freeing paths repeatedly call accept_memory() on ranges ending at the hole
boundary.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113105554.199912-1-pankaj.gupta@amd.com?part=1