[PATCH] x86/mm: ensure L2 is always freed if empty

[PATCH] x86/mm: ensure L2 is always freed if empty

[Roger Pau Monne]

The current logic in modify_xen_mappings() allows for fully empty L2 tables to
not be freed and unhooked from the parent L3 if the last L2 slot is not
populated.

Ensure that even when an L2 slot is empty the logic to check whether the whole
L2 can be removed is not skipped.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I've attempted to find a Fixes tag for this one, but I'm afraid there have been
many changes in the function, and it's possibly the code that introduced the L2
freeing (4376c05c31132) the one that failed to originally adjust this case.
---
 xen/arch/x86/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d537a799bced..0f53dcebad95 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5717,7 +5717,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
 
             v += 1UL << L2_PAGETABLE_SHIFT;
             v &= ~((1UL << L2_PAGETABLE_SHIFT) - 1);
-            continue;
+            goto check_l3;
         }
 
         if ( l2e_get_flags(*pl2e) & _PAGE_PSE )
-- 
2.46.0

Re: [PATCH] x86/mm: ensure L2 is always freed if empty

[Jan Beulich]

On 30.10.2024 12:48, Roger Pau Monne wrote:
> The current logic in modify_xen_mappings() allows for fully empty L2 tables to
> not be freed and unhooked from the parent L3 if the last L2 slot is not
> populated.
> 
> Ensure that even when an L2 slot is empty the logic to check whether the whole
> L2 can be removed is not skipped.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> ---
> I've attempted to find a Fixes tag for this one, but I'm afraid there have been
> many changes in the function, and it's possibly the code that introduced the L2
> freeing (4376c05c31132) the one that failed to originally adjust this case.

Yes, that looks to be the one. It has been a long while, but I think it was
(wrong) similarity considerations with the corresponding L3E handling (near
the top of the main loop) that resulted in me not touching that "continue".
We certainly don't want to ever free L3 tables (vacating L4 entries); since
the 32-bit case still mattered back then, the "continue" also couldn't have
been simply replaced (as there that same consideration applied to L2 tables
and L3 entries, for further extended reasons imposed by PAE).

Jan

Re: [PATCH] x86/mm: ensure L2 is always freed if empty

[Roger Pau Monné]

On Thu, Oct 31, 2024 at 10:59:36AM +0100, Jan Beulich wrote:
> On 30.10.2024 12:48, Roger Pau Monne wrote:
> > The current logic in modify_xen_mappings() allows for fully empty L2 tables to
> > not be freed and unhooked from the parent L3 if the last L2 slot is not
> > populated.
> > 
> > Ensure that even when an L2 slot is empty the logic to check whether the whole
> > L2 can be removed is not skipped.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> > ---
> > I've attempted to find a Fixes tag for this one, but I'm afraid there have been
> > many changes in the function, and it's possibly the code that introduced the L2
> > freeing (4376c05c31132) the one that failed to originally adjust this case.
> 
> Yes, that looks to be the one. It has been a long while, but I think it was
> (wrong) similarity considerations with the corresponding L3E handling (near
> the top of the main loop) that resulted in me not touching that "continue".
> We certainly don't want to ever free L3 tables (vacating L4 entries); since
> the 32-bit case still mattered back then, the "continue" also couldn't have
> been simply replaced (as there that same consideration applied to L2 tables
> and L3 entries, for further extended reasons imposed by PAE).

I agree we don't want to free L3 tables and thus zap L4 entries.

Feel free to add:

Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available')

If you want.

Thanks, Roger.

Re: [PATCH] x86/mm: ensure L2 is always freed if empty

[Jan Beulich]

On 31.10.2024 11:36, Roger Pau Monné wrote:
> On Thu, Oct 31, 2024 at 10:59:36AM +0100, Jan Beulich wrote:
>> On 30.10.2024 12:48, Roger Pau Monne wrote:
>>> The current logic in modify_xen_mappings() allows for fully empty L2 tables to
>>> not be freed and unhooked from the parent L3 if the last L2 slot is not
>>> populated.
>>>
>>> Ensure that even when an L2 slot is empty the logic to check whether the whole
>>> L2 can be removed is not skipped.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>>> ---
>>> I've attempted to find a Fixes tag for this one, but I'm afraid there have been
>>> many changes in the function, and it's possibly the code that introduced the L2
>>> freeing (4376c05c31132) the one that failed to originally adjust this case.
>>
>> Yes, that looks to be the one. It has been a long while, but I think it was
>> (wrong) similarity considerations with the corresponding L3E handling (near
>> the top of the main loop) that resulted in me not touching that "continue".
>> We certainly don't want to ever free L3 tables (vacating L4 entries); since
>> the 32-bit case still mattered back then, the "continue" also couldn't have
>> been simply replaced (as there that same consideration applied to L2 tables
>> and L3 entries, for further extended reasons imposed by PAE).
> 
> I agree we don't want to free L3 tables and thus zap L4 entries.
> 
> Feel free to add:
> 
> Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available')
> 
> If you want.

Done so. Despite the tag I'm uncertain whether to backport.

Jan

Re: [PATCH] x86/mm: ensure L2 is always freed if empty

[Andrew Cooper]

On 30/10/2024 11:48 am, Roger Pau Monne wrote:
> The current logic in modify_xen_mappings() allows for fully empty L2 tables to
> not be freed and unhooked from the parent L3 if the last L2 slot is not
> populated.
>
> Ensure that even when an L2 slot is empty the logic to check whether the whole
> L2 can be removed is not skipped.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I've attempted to find a Fixes tag for this one, but I'm afraid there have been
> many changes in the function, and it's possibly the code that introduced the L2
> freeing (4376c05c31132) the one that failed to originally adjust this case.
> ---
>  xen/arch/x86/mm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d537a799bced..0f53dcebad95 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5717,7 +5717,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
>  
>              v += 1UL << L2_PAGETABLE_SHIFT;
>              v &= ~((1UL << L2_PAGETABLE_SHIFT) - 1);
> -            continue;
> +            goto check_l3;
>          }
>  
>          if ( l2e_get_flags(*pl2e) & _PAGE_PSE )

https://gitlab.com/xen-project/hardware/xen/-/jobs/8240163332

That's hitting the BUG_ON(!nr_bootmem_regions); in alloc_boot_pages().

  [<ffff82d04060407a>] R alloc_boot_pages+0x46/0xbc
  [<ffff82d04033a76f>] S alloc_xen_pagetable+0x5e/0x60
  [<ffff82d04033a7af>] F alloc_mapped_pagetable+0xd/0x3e
  [<ffff82d04033a948>] F arch/x86/mm.c#virt_to_xen_l2e+0x78/0xec
  [<ffff82d04033ba71>] F map_pages_to_xen+0xefb/0x1079
  [<ffff82d04063d55c>] F arch/x86/setup#bootstrap_map_addr+0x95/0xff

I think what's happened is you've "freed" l2_bootmap[], causing
bootstrap_map_addr() to now need to allocate memory.

I've reverted for now, per Roger's request on Matrix.

~Andrew