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

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 [email protected] # 7.2.x

Signed-off-by M. Vefa Bicakci <[email protected]>

---
=== 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(-)

arch/x86/xen/enlighten_pv.c +9 -1

index c5117a7730d6..68f8a977999b 100644
@@ -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)

include/linux/sched.h +12 -0

index 1c93da75226e..3f9341f1f827 100644
@@ -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.

Quoted 128 lines · 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 [email protected] # 7.2.x

Signed-off-by M. Vefa Bicakci <[email protected]>

---
=== 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(-)

arch/x86/xen/enlighten_pv.c +9 -1

index c5117a7730d6..68f8a977999b 100644
@@ -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)

include/linux/sched.h +14 -0

index 1c93da75226e..3f9341f1f827 100644
@@ -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)

include/linux/sched.h static inline bool is_lazy_mmu_mode_active(void) · line 1815

Quoted 7 lines

include/linux/sched.h +5 -0

+{
+    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?

Quoted 129 lines · Juergen Gross

Quoted 128 lines · 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 [email protected] # 7.2.x

Signed-off-by M. Vefa Bicakci <[email protected]>

---
=== 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(-)

arch/x86/xen/enlighten_pv.c +9 -1

index c5117a7730d6..68f8a977999b 100644
@@ -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)

include/linux/sched.h +14 -0

index 1c93da75226e..3f9341f1f827 100644
@@ -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)

Quoted 8 lines

include/linux/sched.h static inline bool is_lazy_mmu_mode_active(void) · line 1815

Quoted 7 lines

include/linux/sched.h +5 -0

+{
+    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.

Thanks for taking the time to review and provide feedback!

Quoted 10 lines

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?

I agree that your proposed approach is indeed more elegant.

To confirm my understanding: Setting every new task's pause count to 1 will ensure that the first context switch into that task will not underflow the pause count. Also, making __task_lazy_mmu_mode_resume() return early with a warning if the pause_count is already zero would guard against encountering the same issue again.

Do you intend to take this on, or would you like me to take a stab at this?