1. Patchew
  2. Xen
  3. View series

[PATCH v1] x86/mm: avoid inadvertently degrading a TLB flush to local only

David Vrabel posted 1 patch 2 years ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220419150320.64783-1-dvrabel@cantab.net
xen/arch/x86/mm.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
[PATCH v1] x86/mm: avoid inadvertently degrading a TLB flush to local only
Posted by David Vrabel 2 years ago 
From: David Vrabel <dvrabel@amazon.co.uk>

If the direct map is incorrectly modified with interrupts disabled,
the required TLB flushes are degraded to flushing the local CPU only.

This could lead to very hard to diagnose problems as different CPUs will
end up with different views of memory. Although, no such issues have yet
been identified.

Change the check in the flush_area() macro to look at system_state
instead. This defers the switch from local to all later in the boot
(see xen/arch/x86/setup.c:__start_xen()). This is fine because
additional PCPUs are not brought up until after the system state is
SYS_STATE_smp_boot.

Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>
---
 xen/arch/x86/mm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c271e383b5..72dbce43b1 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5071,11 +5071,10 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
 #define lNf_to_l1f(f) (((f) & _PAGE_PRESENT) ? ((f) & ~_PAGE_PSE) : (f))
 
 /*
- * map_pages_to_xen() can be called with interrupts disabled during
- * early bootstrap. In this case it is safe to use flush_area_local()
- * and avoid locking because only the local CPU is online.
+ * map_pages_to_xen() can be called early in boot before any other
+ * CPUs are online. Use flush_area_local() in this case.
  */
-#define flush_area(v,f) (!local_irq_is_enabled() ?              \
+#define flush_area(v,f) (system_state < SYS_STATE_smp_boot ?    \
                          flush_area_local((const void *)v, f) : \
                          flush_area_all((const void *)v, f))
 
-- 
2.30.2
Regression with CET: [PATCH v1] x86/mm: avoid inadvertently degrading a TLB flush to local only
Posted by Andrew Cooper 2 years ago 
On 19/04/2022 16:03, David Vrabel wrote:
> From: David Vrabel <dvrabel@amazon.co.uk>
>
> If the direct map is incorrectly modified with interrupts disabled,
> the required TLB flushes are degraded to flushing the local CPU only.
>
> This could lead to very hard to diagnose problems as different CPUs will
> end up with different views of memory. Although, no such issues have yet
> been identified.
>
> Change the check in the flush_area() macro to look at system_state
> instead. This defers the switch from local to all later in the boot
> (see xen/arch/x86/setup.c:__start_xen()). This is fine because
> additional PCPUs are not brought up until after the system state is
> SYS_STATE_smp_boot.
>
> Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>

This explodes on CET systems:

(XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
(XEN) ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
<snip>
(XEN) Xen call trace:
(XEN)    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
(XEN)    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
(XEN)    [<ffff82d0404474f9>] F
arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
(XEN)    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
(XEN)    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
(XEN)    [<ffff82d040203344>] F __high_start+0x94/0xa0
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
(XEN) ****************************************
(XEN)

We really did want a local-only flush here, because we specifically
intended to make self-modifying changes before bringing secondary CPUs up.

~Andrew
Re: Regression with CET: [PATCH v1] x86/mm: avoid inadvertently degrading a TLB flush to local only
Posted by David Vrabel 2 years ago 

On 27/04/2022 19:03, Andrew Cooper wrote:
> On 19/04/2022 16:03, David Vrabel wrote:
>> From: David Vrabel <dvrabel@amazon.co.uk>
>>
>> If the direct map is incorrectly modified with interrupts disabled,
>> the required TLB flushes are degraded to flushing the local CPU only.
>>
>> This could lead to very hard to diagnose problems as different CPUs will
>> end up with different views of memory. Although, no such issues have yet
>> been identified.
>>
>> Change the check in the flush_area() macro to look at system_state
>> instead. This defers the switch from local to all later in the boot
>> (see xen/arch/x86/setup.c:__start_xen()). This is fine because
>> additional PCPUs are not brought up until after the system state is
>> SYS_STATE_smp_boot.
>>
>> Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>
> 
> This explodes on CET systems:
> 
> (XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
> (XEN) ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
> (XEN) CPU:    0
> (XEN) RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
> <snip>
> (XEN) Xen call trace:
> (XEN)    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
> (XEN)    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
> (XEN)    [<ffff82d0404474f9>] F
> arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
> (XEN)    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
> (XEN)    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
> (XEN)    [<ffff82d040203344>] F __high_start+0x94/0xa0
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
> (XEN) ****************************************
> (XEN)
> 
> We really did want a local-only flush here, because we specifically
> intended to make self-modifying changes before bringing secondary CPUs up.

I think the transition to SYS_STATE_smp_boot system state should be 
later. i.e., the last point were only 1 PCPU is guaranteed.

David
Re: Regression with CET: [PATCH v1] x86/mm: avoid inadvertently degrading a TLB flush to local only
Posted by Jan Beulich 2 years ago 
On 27.04.2022 20:44, David Vrabel wrote:
> 
> 
> On 27/04/2022 19:03, Andrew Cooper wrote:
>> On 19/04/2022 16:03, David Vrabel wrote:
>>> From: David Vrabel <dvrabel@amazon.co.uk>
>>>
>>> If the direct map is incorrectly modified with interrupts disabled,
>>> the required TLB flushes are degraded to flushing the local CPU only.
>>>
>>> This could lead to very hard to diagnose problems as different CPUs will
>>> end up with different views of memory. Although, no such issues have yet
>>> been identified.
>>>
>>> Change the check in the flush_area() macro to look at system_state
>>> instead. This defers the switch from local to all later in the boot
>>> (see xen/arch/x86/setup.c:__start_xen()). This is fine because
>>> additional PCPUs are not brought up until after the system state is
>>> SYS_STATE_smp_boot.
>>>
>>> Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>
>>
>> This explodes on CET systems:
>>
>> (XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
>> (XEN) ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
>> (XEN) CPU:    0
>> (XEN) RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
>> <snip>
>> (XEN) Xen call trace:
>> (XEN)    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
>> (XEN)    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
>> (XEN)    [<ffff82d0404474f9>] F
>> arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
>> (XEN)    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
>> (XEN)    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
>> (XEN)    [<ffff82d040203344>] F __high_start+0x94/0xa0
>> (XEN)
>> (XEN)
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
>> (XEN) ****************************************
>> (XEN)
>>
>> We really did want a local-only flush here, because we specifically
>> intended to make self-modifying changes before bringing secondary CPUs up.
> 
> I think the transition to SYS_STATE_smp_boot system state should be 
> later. i.e., the last point were only 1 PCPU is guaranteed.

I'm not sure there isn't code which assumes pre-SMP initcalls to happen
in this state already. So it may take addition of yet another state if
no other solution can be found. Additionally, how would you mean to
deal with the PV shim case then, which continues to be running UP quite
a bit further?

Jan
Re: Regression with CET: [PATCH v1] x86/mm: avoid inadvertently degrading a TLB flush to local only
Posted by Jan Beulich 2 years ago 
On 28.04.2022 09:37, Jan Beulich wrote:
> On 27.04.2022 20:44, David Vrabel wrote:
>>
>>
>> On 27/04/2022 19:03, Andrew Cooper wrote:
>>> On 19/04/2022 16:03, David Vrabel wrote:
>>>> From: David Vrabel <dvrabel@amazon.co.uk>
>>>>
>>>> If the direct map is incorrectly modified with interrupts disabled,
>>>> the required TLB flushes are degraded to flushing the local CPU only.
>>>>
>>>> This could lead to very hard to diagnose problems as different CPUs will
>>>> end up with different views of memory. Although, no such issues have yet
>>>> been identified.
>>>>
>>>> Change the check in the flush_area() macro to look at system_state
>>>> instead. This defers the switch from local to all later in the boot
>>>> (see xen/arch/x86/setup.c:__start_xen()). This is fine because
>>>> additional PCPUs are not brought up until after the system state is
>>>> SYS_STATE_smp_boot.
>>>>
>>>> Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>
>>>
>>> This explodes on CET systems:
>>>
>>> (XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
>>> (XEN) ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
>>> (XEN) CPU:    0
>>> (XEN) RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
>>> <snip>
>>> (XEN) Xen call trace:
>>> (XEN)    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
>>> (XEN)    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
>>> (XEN)    [<ffff82d0404474f9>] F
>>> arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
>>> (XEN)    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
>>> (XEN)    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
>>> (XEN)    [<ffff82d040203344>] F __high_start+0x94/0xa0
>>> (XEN)
>>> (XEN)
>>> (XEN) ****************************************
>>> (XEN) Panic on CPU 0:
>>> (XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
>>> (XEN) ****************************************
>>> (XEN)
>>>
>>> We really did want a local-only flush here, because we specifically
>>> intended to make self-modifying changes before bringing secondary CPUs up.
>>
>> I think the transition to SYS_STATE_smp_boot system state should be 
>> later. i.e., the last point were only 1 PCPU is guaranteed.
> 
> I'm not sure there isn't code which assumes pre-SMP initcalls to happen
> in this state already. So it may take addition of yet another state if
> no other solution can be found.

Or maybe this again shouldn't be using system_state but num_online_cpus()?

Jan
Re: [PATCH v1] x86/mm: avoid inadvertently degrading a TLB flush to local only
Posted by Jan Beulich 2 years ago 
On 19.04.2022 17:03, David Vrabel wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5071,11 +5071,10 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
>  #define lNf_to_l1f(f) (((f) & _PAGE_PRESENT) ? ((f) & ~_PAGE_PSE) : (f))
>  
>  /*
> - * map_pages_to_xen() can be called with interrupts disabled during
> - * early bootstrap. In this case it is safe to use flush_area_local()
> - * and avoid locking because only the local CPU is online.
> + * map_pages_to_xen() can be called early in boot before any other
> + * CPUs are online. Use flush_area_local() in this case.
>   */
> -#define flush_area(v,f) (!local_irq_is_enabled() ?              \
> +#define flush_area(v,f) (system_state < SYS_STATE_smp_boot ?    \
>                           flush_area_local((const void *)v, f) : \
>                           flush_area_all((const void *)v, f))
>  

I agree with the change, but I wonder whether it wouldn't better be
accompanied by an assertion proving that IRQs are enabled. But wait -
flush_area_mask() has such an assertion, so all is fine.

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

Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.