[PATCH v3] x86/traps: Correct pagefault handling issues introduced in c/s d5c251c

[PATCH v3] x86/traps: Correct pagefault handling issues introduced in c/s d5c251c

[Andrew Cooper]

There are two bugs.

Firstly, the ASSERT(paging_mode_only_log_dirty(d)) can trip when servicing a
hypervisor #PF in the context of an HVM guest, e.g. a copy_to_user() failure
in the shadow pagetable code.

Secondly, the entry conditions paging_fault() were previously guarded on
!paging_mode_external(d) which limited entry to PV contexts, but for both
guest and hypervisor faults.  Switching this to paging_mode_log_dirty() opened
it up to HVM contexts as well.

Reinstate the old !paging_mode_external(d) check, as it is actually the
relevent fact, and extend the comment to explicitly state that hypervisor
faults should follow this path.

Inside, we are now guarenteed to be in the context of a PV guest, so can
safely use the assertion about log dirty.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

v3:
 * Rework, to fix it properly.
---
 xen/arch/x86/traps.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 2d79ee0..d69c02d 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1797,10 +1797,6 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
     if ( in_irq() || !(regs->eflags & X86_EFLAGS_IF) )
         return 0;
 
-    /* Logdirty mode is the only expected paging mode for PV guests. */
-    if ( paging_mode_enabled(d) )
-        ASSERT(paging_mode_only_log_dirty(d));
-
     if ( !(regs->error_code & PFEC_page_present) &&
           (pagefault_by_memadd(addr, regs)) )
         return handle_memadd_fault(addr, regs);
@@ -1831,10 +1827,19 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
             return EXCRET_fault_fixed;
     }
 
-    /* Logdirty guests call back into the paging code to update shadows. */
-    if ( paging_mode_log_dirty(d) )
+    /*
+     * For non-external shadowed guests, we fix up both their own pagefaults
+     * and Xen's, since they share the pagetables.  This includes hypervisor
+     * faults, e.g. from copy_to_user().
+     */
+    if ( paging_mode_enabled(d) && !paging_mode_external(d) )
     {
-        int ret = paging_fault(addr, regs);
+        int ret;
+
+        /* Logdirty mode is the only expected paging mode for PV guests. */
+        ASSERT(paging_mode_only_log_dirty(d));
+
+        ret = paging_fault(addr, regs);
         if ( ret == EXCRET_fault_fixed )
             trace_trap_two_addr(TRC_PV_PAGING_FIXUP, regs->eip, addr);
         return ret;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3] x86/traps: Correct pagefault handling issues introduced in c/s d5c251c

[Tim Deegan]

At 16:53 +0000 on 14 Dec (1481734422), Andrew Cooper wrote:
> There are two bugs.
> 
> Firstly, the ASSERT(paging_mode_only_log_dirty(d)) can trip when servicing a
> hypervisor #PF in the context of an HVM guest, e.g. a copy_to_user() failure
> in the shadow pagetable code.
> 
> Secondly, the entry conditions paging_fault() were previously guarded on
> !paging_mode_external(d) which limited entry to PV contexts, but for both
> guest and hypervisor faults.  Switching this to paging_mode_log_dirty() opened
> it up to HVM contexts as well.
> 
> Reinstate the old !paging_mode_external(d) check, as it is actually the
> relevent fact, and extend the comment to explicitly state that hypervisor
> faults should follow this path.
> 
> Inside, we are now guarenteed to be in the context of a PV guest, so can
> safely use the assertion about log dirty.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Tim Deegan <tim@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3] x86/traps: Correct pagefault handling issues introduced in c/s d5c251c

[Jan Beulich]

>>> On 14.12.16 at 17:53, <andrew.cooper3@citrix.com> wrote:
> There are two bugs.
> 
> Firstly, the ASSERT(paging_mode_only_log_dirty(d)) can trip when servicing a
> hypervisor #PF in the context of an HVM guest, e.g. a copy_to_user() failure
> in the shadow pagetable code.
> 
> Secondly, the entry conditions paging_fault() were previously guarded on
> !paging_mode_external(d) which limited entry to PV contexts, but for both
> guest and hypervisor faults.  Switching this to paging_mode_log_dirty() 
> opened
> it up to HVM contexts as well.
> 
> Reinstate the old !paging_mode_external(d) check, as it is actually the
> relevent fact, and extend the comment to explicitly state that hypervisor
> faults should follow this path.
> 
> Inside, we are now guarenteed to be in the context of a PV guest, so can
> safely use the assertion about log dirty.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel