[PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[Xin Li (Intel)]

The FRED RSP0 MSR (pointing to the top of the kernel stack for user
level event delivery) and its per CPU cache should be kept in sync to
avoid redundant writes in the exit to user space path, as a result,
a write to the FRED RSP0 MSR is paired with a write to its per CPU
cache as fred_update_rsp0() does.

However as the FRED RSP0 MSR is set to 0 in cpu_init_fred_exceptions(),
it gets out of sync with its per CPU cache during a CPU offline/online
cycle, which causes #DF exceptions if no context switch happens after
a CPU offline/online cycle and before exit to user space.

Fix the bug through resynchronizing the FRED RSP0 MSR with its per CPU
cache value in cpu_init_fred_exceptions().

Fixes: fe85ee391966 ("x86/entry: Set FRED RSP0 on return to userspace instead of context switch")
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/fred.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fred.c b/arch/x86/kernel/fred.c
index 8d32c3f48abc..9524ace96bfa 100644
--- a/arch/x86/kernel/fred.c
+++ b/arch/x86/kernel/fred.c
@@ -50,7 +50,18 @@ void cpu_init_fred_exceptions(void)
 	       FRED_CONFIG_ENTRYPOINT(asm_fred_entrypoint_user));
 
 	wrmsrl(MSR_IA32_FRED_STKLVLS, 0);
-	wrmsrl(MSR_IA32_FRED_RSP0, 0);
+
+	/*
+	 * Resynchronize the FRED RSP0 MSR with its per CPU cache value.
+	 *
+	 * Another option is to leave the FRED RSP0 MSR as-is, because the RESET
+	 * state of FRED MSRs is zero and INIT does not change the value of the
+	 * FRED MSRs in a CPU offline/online cycle.  But it doesn't seem safe to
+	 * depend on the properties of INIT as that's way too many things that
+	 * could cause bugs.
+	 */
+	wrmsrl(MSR_IA32_FRED_RSP0, __this_cpu_read(fred_rsp0));
+
 	wrmsrl(MSR_IA32_FRED_RSP1, 0);
 	wrmsrl(MSR_IA32_FRED_RSP2, 0);
 	wrmsrl(MSR_IA32_FRED_RSP3, 0);

base-commit: cf6b067860f69fe01b08ebae9138fe0f89854398
-- 
2.47.1

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[H. Peter Anvin]

On January 7, 2025 6:36:41 PM PST, "Xin Li (Intel)" <xin@zytor.com> wrote:
>The FRED RSP0 MSR (pointing to the top of the kernel stack for user
>level event delivery) and its per CPU cache should be kept in sync to
>avoid redundant writes in the exit to user space path, as a result,
>a write to the FRED RSP0 MSR is paired with a write to its per CPU
>cache as fred_update_rsp0() does.
>
>However as the FRED RSP0 MSR is set to 0 in cpu_init_fred_exceptions(),
>it gets out of sync with its per CPU cache during a CPU offline/online
>cycle, which causes #DF exceptions if no context switch happens after
>a CPU offline/online cycle and before exit to user space.
>
>Fix the bug through resynchronizing the FRED RSP0 MSR with its per CPU
>cache value in cpu_init_fred_exceptions().
>
>Fixes: fe85ee391966 ("x86/entry: Set FRED RSP0 on return to userspace instead of context switch")
>Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>Cc: stable@vger.kernel.org
>---
> arch/x86/kernel/fred.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/kernel/fred.c b/arch/x86/kernel/fred.c
>index 8d32c3f48abc..9524ace96bfa 100644
>--- a/arch/x86/kernel/fred.c
>+++ b/arch/x86/kernel/fred.c
>@@ -50,7 +50,18 @@ void cpu_init_fred_exceptions(void)
> 	       FRED_CONFIG_ENTRYPOINT(asm_fred_entrypoint_user));
> 
> 	wrmsrl(MSR_IA32_FRED_STKLVLS, 0);
>-	wrmsrl(MSR_IA32_FRED_RSP0, 0);
>+
>+	/*
>+	 * Resynchronize the FRED RSP0 MSR with its per CPU cache value.
>+	 *
>+	 * Another option is to leave the FRED RSP0 MSR as-is, because the RESET
>+	 * state of FRED MSRs is zero and INIT does not change the value of the
>+	 * FRED MSRs in a CPU offline/online cycle.  But it doesn't seem safe to
>+	 * depend on the properties of INIT as that's way too many things that
>+	 * could cause bugs.
>+	 */
>+	wrmsrl(MSR_IA32_FRED_RSP0, __this_cpu_read(fred_rsp0));
>+
> 	wrmsrl(MSR_IA32_FRED_RSP1, 0);
> 	wrmsrl(MSR_IA32_FRED_RSP2, 0);
> 	wrmsrl(MSR_IA32_FRED_RSP3, 0);
>
>base-commit: cf6b067860f69fe01b08ebae9138fe0f89854398

Looks straightforward enough.

Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[Dave Hansen]

On 1/7/25 18:36, Xin Li (Intel) wrote:
> +	 * Another option is to leave the FRED RSP0 MSR as-is, because the RESET
> +	 * state of FRED MSRs is zero and INIT does not change the value of the
> +	 * FRED MSRs in a CPU offline/online cycle.  But it doesn't seem safe to
> +	 * depend on the properties of INIT as that's way too many things that
> +	 * could cause bugs.

Wouldn't that also open us up to kexec problems? If the last kernel
(maybe not even Linux) left RSP0 set, then the current kernel might
eventually try to run userspace with the old kernel's RSP0 value.

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[Andrew Cooper]

On 08/01/2025 8:14 pm, Dave Hansen wrote:
> On 1/7/25 18:36, Xin Li (Intel) wrote:
>> +	 * Another option is to leave the FRED RSP0 MSR as-is, because the RESET
>> +	 * state of FRED MSRs is zero and INIT does not change the value of the
>> +	 * FRED MSRs in a CPU offline/online cycle.  But it doesn't seem safe to
>> +	 * depend on the properties of INIT as that's way too many things that
>> +	 * could cause bugs.
> Wouldn't that also open us up to kexec problems? If the last kernel
> (maybe not even Linux) left RSP0 set, then the current kernel might
> eventually try to run userspace with the old kernel's RSP0 value.

That's why the init code needs to write every control MSR, even if it's
filling in 0 because it doesn't want to use the feature.

The job of the init code is to go from an unknown state into a good
state, not from the INIT/#RESET state.

Or someone could finally write down an ABI for Kexec which details who's
responsibility it is :)

~Andrew

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[Dave Hansen]

On 1/8/25 13:10, Andrew Cooper wrote:
> On 08/01/2025 8:14 pm, Dave Hansen wrote:
>> On 1/7/25 18:36, Xin Li (Intel) wrote:
>>> +	 * Another option is to leave the FRED RSP0 MSR as-is, because the RESET
>>> +	 * state of FRED MSRs is zero and INIT does not change the value of the
>>> +	 * FRED MSRs in a CPU offline/online cycle.  But it doesn't seem safe to
>>> +	 * depend on the properties of INIT as that's way too many things that
>>> +	 * could cause bugs.
>> Wouldn't that also open us up to kexec problems? If the last kernel
>> (maybe not even Linux) left RSP0 set, then the current kernel might
>> eventually try to run userspace with the old kernel's RSP0 value.
> That's why the init code needs to write every control MSR, even if it's
> filling in 0 because it doesn't want to use the feature.
> 
> The job of the init code is to go from an unknown state into a good
> state, not from the INIT/#RESET state.

Right, so I think the quoted "Another option..." comment needs to get
zapped. Because it's not really another option. Discussing alternatives
is better left for changelogs anyway, not code comments.

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[Xin Li]

On 1/8/2025 2:31 PM, Dave Hansen wrote:
> On 1/8/25 13:10, Andrew Cooper wrote:
>> On 08/01/2025 8:14 pm, Dave Hansen wrote:
>>> On 1/7/25 18:36, Xin Li (Intel) wrote:
>>>> +	 * Another option is to leave the FRED RSP0 MSR as-is, because the RESET
>>>> +	 * state of FRED MSRs is zero and INIT does not change the value of the
>>>> +	 * FRED MSRs in a CPU offline/online cycle.  But it doesn't seem safe to
>>>> +	 * depend on the properties of INIT as that's way too many things that
>>>> +	 * could cause bugs.
>>> Wouldn't that also open us up to kexec problems? If the last kernel
>>> (maybe not even Linux) left RSP0 set, then the current kernel might
>>> eventually try to run userspace with the old kernel's RSP0 value.
>> That's why the init code needs to write every control MSR, even if it's
>> filling in 0 because it doesn't want to use the feature.
>>
>> The job of the init code is to go from an unknown state into a good
>> state, not from the INIT/#RESET state.
> 
> Right, so I think the quoted "Another option..." comment needs to get
> zapped. Because it's not really another option. Discussing alternatives
> is better left for changelogs anyway, not code comments.
> 

Will do!

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[H. Peter Anvin]

On January 8, 2025 12:14:18 PM PST, Dave Hansen <dave.hansen@intel.com> wrote:
>On 1/7/25 18:36, Xin Li (Intel) wrote:
>> +	 * Another option is to leave the FRED RSP0 MSR as-is, because the RESET
>> +	 * state of FRED MSRs is zero and INIT does not change the value of the
>> +	 * FRED MSRs in a CPU offline/online cycle.  But it doesn't seem safe to
>> +	 * depend on the properties of INIT as that's way too many things that
>> +	 * could cause bugs.
>
>Wouldn't that also open us up to kexec problems? If the last kernel
>(maybe not even Linux) left RSP0 set, then the current kernel might
>eventually try to run userspace with the old kernel's RSP0 value.
>

Yeah, it is a nonstarter. I believe Xin was trying to document the problems with *not* doing this, so someone doesn't have the bright idea to "optimize" this later.

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[Xin Li]

On 1/9/2025 1:46 AM, H. Peter Anvin wrote:
> On January 8, 2025 12:14:18 PM PST, Dave Hansen <dave.hansen@intel.com> wrote:
>> On 1/7/25 18:36, Xin Li (Intel) wrote:
>>> +	 * Another option is to leave the FRED RSP0 MSR as-is, because the RESET
>>> +	 * state of FRED MSRs is zero and INIT does not change the value of the
>>> +	 * FRED MSRs in a CPU offline/online cycle.  But it doesn't seem safe to
>>> +	 * depend on the properties of INIT as that's way too many things that
>>> +	 * could cause bugs.
>>
>> Wouldn't that also open us up to kexec problems? If the last kernel
>> (maybe not even Linux) left RSP0 set, then the current kernel might
>> eventually try to run userspace with the old kernel's RSP0 value.
>>
> 
> Yeah, it is a nonstarter. I believe Xin was trying to document the problems with *not* doing this, so someone doesn't have the bright idea to "optimize" this later.
> 

exactly ;)

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[Dave Hansen]

On 1/7/25 18:36, Xin Li (Intel) wrote:
> The FRED RSP0 MSR (pointing to the top of the kernel stack for user
> level event delivery) and its per CPU cache should be kept in sync to
> avoid redundant writes in the exit to user space path, as a result,
> a write to the FRED RSP0 MSR is paired with a write to its per CPU
> cache as fred_update_rsp0() does.

I _think_ you're trying to explain the general use of a per-cpu MSR
cache. That's good. But I was reading this paragraph and thinking at
this point that the bug had something to do with redundant writes to the
MSR.

How about this?

	The FRED RSP0 MSR is only used for delivering events when
	running userspace. This kernel leverages this property to reduce
	expensive MSR writes and optimize context switches.  The kernel
	only writes the MSR when about to run userspace *and* when the
	MSR has actually changed since the last time userspace ran.

	This optimization is implemented by maintaining a per-cpu cache
	of FRED RSP0 and then checking that against the value for the
	current task's stack before running userspace.

	However cpu_init_fred_exceptions() writes the MSR without
	updating the per-cpu cache. This means that the kernel might
	return to userspace with MSR_IA32_FRED_RSP0==0 when it needed to
	point to the current task stack. This would induce a double
	fault (#DF), which is bad.

	A context switch after cpu_init_fred_exceptions() can paper over
	the issue since it updates the cached value. That evidently
	happens most of the time explaining how this bug got through.

Re: [PATCH v1 1/1] x86/fred: Fix the FRED RSP0 MSR out of sync with its per CPU cache

[Xin Li]

On 1/8/2025 3:04 PM, Dave Hansen wrote:
> On 1/7/25 18:36, Xin Li (Intel) wrote:
>> The FRED RSP0 MSR (pointing to the top of the kernel stack for user
>> level event delivery) and its per CPU cache should be kept in sync to
>> avoid redundant writes in the exit to user space path, as a result,
>> a write to the FRED RSP0 MSR is paired with a write to its per CPU
>> cache as fred_update_rsp0() does.
> 
> I _think_ you're trying to explain the general use of a per-cpu MSR
> cache. That's good. But I was reading this paragraph and thinking at
> this point that the bug had something to do with redundant writes to the
> MSR.
> 
> How about this?
> 
> 	The FRED RSP0 MSR is only used for delivering events when
> 	running userspace. This kernel leverages this property to reduce
> 	expensive MSR writes and optimize context switches.  The kernel
> 	only writes the MSR when about to run userspace *and* when the
> 	MSR has actually changed since the last time userspace ran.
> 
> 	This optimization is implemented by maintaining a per-cpu cache
> 	of FRED RSP0 and then checking that against the value for the
> 	current task's stack before running userspace.
> 
> 	However cpu_init_fred_exceptions() writes the MSR without
> 	updating the per-cpu cache. This means that the kernel might
> 	return to userspace with MSR_IA32_FRED_RSP0==0 when it needed to
> 	point to the current task stack. This would induce a double
> 	fault (#DF), which is bad.
> 
> 	A context switch after cpu_init_fred_exceptions() can paper over
> 	the issue since it updates the cached value. That evidently
> 	happens most of the time explaining how this bug got through.
> 

This is a way better explanation, thanks a lot!