Re: [PATCH] x86/xen: Fix a potential problem in xen_e820_resolve_conflicts()

[Jan Beulich <jbeulich@suse.com>]

On 05.05.2026 11:13, Jürgen Groß wrote:
> On 05.05.26 10:43, Jan Beulich wrote:
>> On 05.05.2026 10:06, Juergen Gross wrote:
>>> When fixing a conflict in xen_e820_resolve_conflicts(), the loop over
>>> the E820 map entries needs to be restarted, as the E820 map will have
>>> been modified by the fix. Otherwise entries might be skipped by
>>> accident.
>>>
>>> Fixes: be35d91c8880 ("xen: tolerate ACPI NVS memory overlapping with Xen allocated memory")
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> First, while trying to review this, isn't there another issue in
>> xen_e820_swap_entry_with_ram(), in that
>>
>> 			entry->addr = entry_end - swap_size +
>> 				      swap_addr - swap_entry->addr;
>>
>>
>> really means to be
>>
>> 			entry->addr = entry_end - swap_size +
>> 				      swap_entry->addr - swap_addr;
>>
>> (affecting non-page-aligned E820 entries)?
> 
> Yes, you are right.
> 
>>
>> Further, that function converts swap_entry to the page-aligned superset
>> of the passed in range. How is it guaranteed that this new range won't
>> overlap with the predecessor and/or successor one? Wouldn't that need
>> to be conversion to the page-aligned subset instead?
> 
> This is subtle. :-)
> 
> We are converting to RAM (usable), so the type value is 1. e820__update_table()
> will handle overlaps just fine, with higher type values "winning" against lower
> ones. So any other region overlapping with the new RAM region will result in
> another conflict in the next loop iteration.

Oh, wow, and this is a property of the function that one can rely upon?

> Using the page-aligned subset would result in possible memory holes, which would
> be problematic (the kernel or page tables shouldn't have holes, after all).

Aren't such holes normal to occur, e.g. on misaligned RAM/UNUSABLE
boundaries?

>> And then, is passing the page-aligned superset to xen_add_remap_nonram()
>> really appropriate? Why would any leading or trailing space there be
>> subject to remapping?
> 
> How would you want to remap a sub-page physical memory area to another location
> without affecting the rest of the page? We are reworking the final p2m map here.

Well, first and foremost: xen_add_remap_nonram() takes and stores byte-
granular addresses / sizes, with the sole requirement being that the
offset-into-page be identical between both addresses. That check alone
already indicates that non-page-aligned addresses are expected to be
passed into here.

Further, xen_acpi_os_ioremap() uses the resulting remap table, and is
byte granular. With the physical address adjustment there, both mappings
could (theoretically) coexist. But the problem I'm trying to point out
is that by passing the page-aligned superset into xen_add_remap_nonram()
you misguide xen_acpi_os_ioremap() (while at the same time
xen_do_remap_nonram() will do suitable rounding to page boundaries even
if exact addresses were passed).

Jan