[PATCH] x86/xen: Fix lazy MMU pause count wrap-around

[PATCH] x86/xen: Fix lazy MMU pause count wrap-around

[M. Vefa Bicakci]

Commit bec6f41a6fe5 ("x86/xen: Get rid of last XEN_LAZY_MMU uses")
unintentionally prevents the activation of lazy MMU mode for most tasks
running on Xen PV VMs (including dom0).

That commit made xen_end_context_switch() call
__task_lazy_mmu_mode_resume() with the task being switched to. However,
the latter function decrements the pause_count field of the task without
checking whether that task's lazy MMU mode was paused or not. A task
being switched to for the first time has never been switched from, so
attempting to resume lazy MMU mode can wrap-around (underflow) the
pause_count from 0 to 255 (U8_MAX).

Furthermore, the non-zero pause_count is copied to child tasks at fork,
so all descendants (including the idle tasks of the non-boot CPUs)
inherit the non-zero pause_count. This prevents enabling lazy MMU mode
for them, because lazy_mmu_mode_enable() does not do anything when
pause_count is non-zero.

Hence, call __task_lazy_mmu_mode_resume() from xen_end_context_switch()
only if lazy MMU mode was paused.

This issue was discovered with CONFIG_DEBUG_VM=y, which enables
pause_count underflow and overflow warnings. In the examples quoted
below, the first warning shows the underflow with PID 1. The second
warning shows the overflow from 255 (U8_MAX) to 0, also for PID 1, as
indicated by the same RDI register value.

Example pause_count underflow warning:

  WARNING: include/linux/pgtable.h:362 at xen_end_context_switch+0x4e/0x60, CPU#0: swapper/0/0
  CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 7.2.4-1 #2 PREEMPT(full)
  RIP: e030:xen_end_context_switch+0x4e/0x60
  ...
  RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff888004930000

Note: The PID is shown as 0 above, because current_task is updated in
__switch_to() after arch_end_context_switch(). arch_end_context_switch()
calls xen_end_context_switch() indirectly.

Example pause_count overflow warning:

  WARNING: include/linux/pgtable.h:319 at xen_start_context_switch+0x64/0x70, CPU#0: swapper/0/1
  CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W  7.2.4-1 #2 PREEMPT(full)
  ...
  RIP: e030:xen_start_context_switch+0x64/0x70
  ...
  RDX: 0000000000000000 RSI: 0000000013f7894d RDI: ffff888004930000
  ...
  Call Trace:
   <TASK>
   __schedule+0x36a/0x1a90
   schedule+0x27/0xa0
   schedule_timeout+0xbd/0x100
   __wait_for_common+0x97/0x1b0
   wait_for_completion_killable+0x24/0x40
   __kthread_create_on_node+0x13e/0x240
   kthread_create_worker_on_node+0xee/0x1c0
   workqueue_init+0x26/0x420
   kernel_init_freeable+0x101/0x360
   kernel_init+0x1a/0x130
   ret_from_fork+0x2b0/0x340
   ret_from_fork_asm+0x1a/0x30
   </TASK>

Fixes: bec6f41a6fe5 ("x86/xen: Get rid of last XEN_LAZY_MMU uses")
Cc: stable@vger.kernel.org # 7.2.x
Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
---
=== Testing notes ===
This commit was tested with kernel v7.3-16755-gd135ab453a74 and v7.4-rc1
with a domU PV VM, and versions from v7.2.4 to v7.2.14 with dom0 and
domU PV VMs running on Xen v4.19.5. CONFIG_DEBUG_VM=y was set for all
tests, and no warnings were observed.

=== Debugging notes ===
For debugging, I instrumented xen_start_context_switch(),
xen_end_context_switch() and copy_process(), and I confirmed that the
inherited pause_count value is decremented for each level of the process
tree, starting from 255.

=== Credits ===
The patch was hand-written, but the commit message was polished with a
few rounds of LLM review.
---
 arch/x86/xen/enlighten_pv.c | 10 +++++++++-
 include/linux/sched.h       | 19 +++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index c5117a7730d6..68f8a977999b 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -431,7 +431,15 @@ static void xen_end_context_switch(struct task_struct *next)
 
 	xen_mc_flush();
 	this_cpu_write(xen_cpu_lazy_mode, false);
-	__task_lazy_mmu_mode_resume(next);
+
+	/*
+	 * A newly created task can be switched to without ever having been
+	 * switched from, so it will not have a prior lazy MMU pause to undo
+	 * from xen_start_context_switch(). Hence, resume lazy MMU only if it
+	 * was paused.
+	 */
+	if (__task_lazy_mmu_mode_paused(next))
+		__task_lazy_mmu_mode_resume(next);
 }
 
 static unsigned long xen_store_tr(void)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 1c93da75226e..3f9341f1f827 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1791,6 +1791,25 @@ static inline bool is_lazy_mmu_mode_active(void)
 
 	return __task_lazy_mmu_mode_active(current);
 }
+
+/**
+ * __task_lazy_mmu_mode_paused() - Return a task's lazy MMU mode pause state
+ * @tsk: The task to check.
+ *
+ * Test whether @tsk has its lazy MMU mode state set to paused
+ * (i.e., pause_count > 0).
+ *
+ * This function is intended for architectures that implement the lazy MMU
+ * mode; it must not be called from generic code.
+ *
+ * Return: Whether task @tsk has paused lazy MMU mode.
+ */
+static inline bool __task_lazy_mmu_mode_paused(struct task_struct *tsk)
+{
+	struct lazy_mmu_state *state = &tsk->lazy_mmu_state;
+
+	return state->pause_count > 0;
+}
 #endif
 
 extern struct pid __rcu *cad_pid;

---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261027-xen-lazy-mmu-78a368cac315

Best regards,
--  
M. Vefa Bicakci <m.v.b@runbox.com>

Re: [PATCH] x86/xen: Fix lazy MMU pause count wrap-around

[Juergen Gross]

[-- Attachment #1.1: Type: text/plain, Size: 6262 bytes --]

On 02.11.26 16:31, M. Vefa Bicakci wrote:
> Commit bec6f41a6fe5 ("x86/xen: Get rid of last XEN_LAZY_MMU uses")
> unintentionally prevents the activation of lazy MMU mode for most tasks
> running on Xen PV VMs (including dom0).
> 
> That commit made xen_end_context_switch() call
> __task_lazy_mmu_mode_resume() with the task being switched to. However,
> the latter function decrements the pause_count field of the task without
> checking whether that task's lazy MMU mode was paused or not. A task
> being switched to for the first time has never been switched from, so
> attempting to resume lazy MMU mode can wrap-around (underflow) the
> pause_count from 0 to 255 (U8_MAX).
> 
> Furthermore, the non-zero pause_count is copied to child tasks at fork,
> so all descendants (including the idle tasks of the non-boot CPUs)
> inherit the non-zero pause_count. This prevents enabling lazy MMU mode
> for them, because lazy_mmu_mode_enable() does not do anything when
> pause_count is non-zero.
> 
> Hence, call __task_lazy_mmu_mode_resume() from xen_end_context_switch()
> only if lazy MMU mode was paused.
> 
> This issue was discovered with CONFIG_DEBUG_VM=y, which enables
> pause_count underflow and overflow warnings. In the examples quoted
> below, the first warning shows the underflow with PID 1. The second
> warning shows the overflow from 255 (U8_MAX) to 0, also for PID 1, as
> indicated by the same RDI register value.
> 
> Example pause_count underflow warning:
> 
>    WARNING: include/linux/pgtable.h:362 at xen_end_context_switch+0x4e/0x60, CPU#0: swapper/0/0
>    CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 7.2.4-1 #2 PREEMPT(full)
>    RIP: e030:xen_end_context_switch+0x4e/0x60
>    ...
>    RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff888004930000
> 
> Note: The PID is shown as 0 above, because current_task is updated in
> __switch_to() after arch_end_context_switch(). arch_end_context_switch()
> calls xen_end_context_switch() indirectly.
> 
> Example pause_count overflow warning:
> 
>    WARNING: include/linux/pgtable.h:319 at xen_start_context_switch+0x64/0x70, CPU#0: swapper/0/1
>    CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W  7.2.4-1 #2 PREEMPT(full)
>    ...
>    RIP: e030:xen_start_context_switch+0x64/0x70
>    ...
>    RDX: 0000000000000000 RSI: 0000000013f7894d RDI: ffff888004930000
>    ...
>    Call Trace:
>     <TASK>
>     __schedule+0x36a/0x1a90
>     schedule+0x27/0xa0
>     schedule_timeout+0xbd/0x100
>     __wait_for_common+0x97/0x1b0
>     wait_for_completion_killable+0x24/0x40
>     __kthread_create_on_node+0x13e/0x240
>     kthread_create_worker_on_node+0xee/0x1c0
>     workqueue_init+0x26/0x420
>     kernel_init_freeable+0x101/0x360
>     kernel_init+0x1a/0x130
>     ret_from_fork+0x2b0/0x340
>     ret_from_fork_asm+0x1a/0x30
>     </TASK>
> 
> Fixes: bec6f41a6fe5 ("x86/xen: Get rid of last XEN_LAZY_MMU uses")
> Cc: stable@vger.kernel.org # 7.2.x
> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
> ---
> === Testing notes ===
> This commit was tested with kernel v7.3-16755-gd135ab453a74 and v7.4-rc1
> with a domU PV VM, and versions from v7.2.4 to v7.2.14 with dom0 and
> domU PV VMs running on Xen v4.19.5. CONFIG_DEBUG_VM=y was set for all
> tests, and no warnings were observed.
> 
> === Debugging notes ===
> For debugging, I instrumented xen_start_context_switch(),
> xen_end_context_switch() and copy_process(), and I confirmed that the
> inherited pause_count value is decremented for each level of the process
> tree, starting from 255.
> 
> === Credits ===
> The patch was hand-written, but the commit message was polished with a
> few rounds of LLM review.
> ---
>   arch/x86/xen/enlighten_pv.c | 10 +++++++++-
>   include/linux/sched.h       | 19 +++++++++++++++++++
>   2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index c5117a7730d6..68f8a977999b 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -431,7 +431,15 @@ static void xen_end_context_switch(struct task_struct *next)
>   
>   	xen_mc_flush();
>   	this_cpu_write(xen_cpu_lazy_mode, false);
> -	__task_lazy_mmu_mode_resume(next);
> +
> +	/*
> +	 * A newly created task can be switched to without ever having been
> +	 * switched from, so it will not have a prior lazy MMU pause to undo
> +	 * from xen_start_context_switch(). Hence, resume lazy MMU only if it
> +	 * was paused.
> +	 */
> +	if (__task_lazy_mmu_mode_paused(next))
> +		__task_lazy_mmu_mode_resume(next);
>   }
>   
>   static unsigned long xen_store_tr(void)
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 1c93da75226e..3f9341f1f827 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1791,6 +1791,25 @@ static inline bool is_lazy_mmu_mode_active(void)
>   
>   	return __task_lazy_mmu_mode_active(current);
>   }
> +
> +/**
> + * __task_lazy_mmu_mode_paused() - Return a task's lazy MMU mode pause state
> + * @tsk: The task to check.
> + *
> + * Test whether @tsk has its lazy MMU mode state set to paused
> + * (i.e., pause_count > 0).
> + *
> + * This function is intended for architectures that implement the lazy MMU
> + * mode; it must not be called from generic code.
> + *
> + * Return: Whether task @tsk has paused lazy MMU mode.
> + */
> +static inline bool __task_lazy_mmu_mode_paused(struct task_struct *tsk)
> +{
> +	struct lazy_mmu_state *state = &tsk->lazy_mmu_state;
> +
> +	return state->pause_count > 0;
> +}
>   #endif
>   
>   extern struct pid __rcu *cad_pid;
Thanks for finding this issue.

While your patch is fixing the problem, I'd prefer fixing the root cause:
When setting up a new struct task_struct, the initial lazy_mmu_state state
should be initialized accordingly. This would require another lazy_mmu_state
hook for initializing it, probably via a simple static_call(). Xen could use
that for setting the pause count to 1.

At the same time __task_lazy_mmu_mode_resume() should probably WARN() if
the pause count is 0 already and NOT decrement the pause count.

Thoughts?


Juergen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]