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

[Jürgen Groß <jgross@xxxxxxxx>]

On 09.11.26 19:42, M. Vefa Bicakci wrote:
> On 2026-11-09 01:40, Juergen Gross wrote:
>
> > 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@xxxxxxxxxxxxxxx # 7.2.x
> > > Signed-off-by: M. Vefa Bicakci <m.v.b@xxxxxxxxxx>
> > > ---
> > > === 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.
> Thanks for taking the time to review and provide feedback!
>
> > 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?
I'll take it, thanks for the offering.


Juergen

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature