Re: [PATCH v2] x86/traps: Re-enable interrupts after reading cr2 in the #PF handler

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Fri Sep 20, 2024 at 3:12 PM BST, Roger Pau Monné wrote:
> On Wed, Sep 18, 2024 at 02:05:54PM +0100, Alejandro Vallejo wrote:
> > Moves sti directly after the cr2 read and immediately after the #PF
> > handler.
>
> I think you need to add some context about why this is needed, iow:
> avoid corrupting %cr2 if a nested 3PF happens.

I can send a v3 with:

```
  Hitting a page fault clobbers %cr2, so if a page fault is handled while
  handling a previous page fault then %cr2 will hold the address of the latter
  fault rather than the former. This patch makes the page fault path delay
  re-enabling IRQs until %cr2 has been read in order to ensure it stays
  consistent.

  Furthermore, the patch preserves the invariant of "IRQs are only re-enabled
  if they were enabled in the interrupted context" in order to not break
  IRQs-off faulting contexts.
```

>
> > While in the area, remove redundant q suffix to a movq in entry.S
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
>
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks

>
> One nit below.
>
> > ---
> > Got lost alongside other patches. Here's the promised v2.
> > 
> > pipeline: 
> > https://gitlab.com/xen-project/people/agvallejo/xen/-/pipelines/1458699639
> > v1: 
> > https://lore.kernel.org/xen-devel/20240911145823.12066-1-alejandro.vallejo@xxxxxxxxx/
> > 
> > v2:
> >   * (cosmetic), add whitespace after comma
> >   * Added ASSERT(local_irq_is_enabled()) to do_page_fault()
> >   * Only re-enable interrupts if they were enabled in the interrupted
> >     context.
> > ---
> >  xen/arch/x86/traps.c        |  8 ++++++++
> >  xen/arch/x86/x86_64/entry.S | 20 ++++++++++++++++----
> >  2 files changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > index 708136f62558..a9c2c607eb08 100644
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -1600,6 +1600,14 @@ void asmlinkage do_page_fault(struct cpu_user_regs 
> > *regs)
> >  
> >      addr = read_cr2();
> >  
> > +    /*
> > +     * Don't re-enable interrupts if we were running an IRQ-off region when
> > +     * we hit the page fault, or we'll break that code.
> > +     */
> > +    ASSERT(!local_irq_is_enabled());
> > +    if ( regs->flags & X86_EFLAGS_IF )
> > +        local_irq_enable();
> > +
> >      /* fixup_page_fault() might change regs->error_code, so cache it here. 
> > */
> >      error_code = regs->error_code;
> >  
> > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > index b8482de8ee5b..218e5ea85efb 100644
> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -844,9 +844,9 @@ handle_exception_saved:
> >  #elif !defined(CONFIG_PV)
> >          ASSERT_CONTEXT_IS_XEN
> >  #endif /* CONFIG_PV */
> > -        sti
> > -1:      movq  %rsp,%rdi
> > -        movzbl UREGS_entry_vector(%rsp),%eax
> > +.Ldispatch_handlers:
>
> Maybe 'dispatch_exception', since it's only exceptions that are
> handled here? dispatch_handlers seems a bit too generic, but no strong
> opinion.

Sure, anything would be better than "1:"

>
> Thanks, Roger.

Cheers,
Alejandro