[Xen-devel] Re: [PATCH 5/6] Handler m2p table and frametable fault in page faulthandler

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx> 28.06.09 11:27 >>>
>...
>+int handle_memadd_fault(unsigned long addr, struct cpu_user_regs *regs)
>+{
>+    l3_pgentry_t  *pl3e;
>+    l3_pgentry_t l3e;
>+    l2_pgentry_t *pl2e;
>+    l2_pgentry_t l2e, idle_l2e;
>+    unsigned long mfn, cr3;
>+
>+    idle_l2e = idle_pg_table_l2[l2_linear_offset(addr)];
>+    if (!(l2e_get_flags(idle_l2e) & _PAGE_PRESENT))
>+        return 0;
>+
>+    cr3 = read_cr3();
>+    mfn = cr3 >> PAGE_SHIFT;
>+
>+    /*
>+     * No need get page type here and validate checking for xen mapping
>+     */
>+    pl3e = map_domain_page(mfn);
>+    pl3e += (cr3 & 0xFE0UL) >> 3;
>+    l3e = pl3e[3];
>+
>+    if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) )
>+        return 0;
>+
>+    mfn = l3e_get_pfn(l3e);
>+    pl2e = map_domain_page(mfn);
>+
>+    l2e = pl2e[l2_table_offset(addr)];
>+
>+    if (l2e_get_flags(l2e) & _PAGE_PRESENT)
>+        return 0;

You'd also need to check that idle_page_table_l2's entry has _PAGE_PRESENT
set here, otherwise you'd risk live locking the domain on an out-of-current-
bounds M2P access.

Furthermore, I'm unsure forwarding the page fault in case you found the
domain's L2 entry to already have _PAGE_PRESENT set is a good way to
handle things: A racing access on another vCPU of the same guest may just
have managed to install that entry.

Bottom line is, you probably need to exclusively check idle_page_table_l2
here.

>+
>+    memcpy(&pl2e[l2_table_offset(addr)],
>+            &idle_pg_table_l2[l2_linear_offset(addr)],
>+            sizeof(l2_pgentry_t));

Using memcpy for a single page table entry seems odd - why not use a direct
assignment? However, perhaps using memcpy() here is the right thing - to
avoid future faults, you could update the full M2P related part of the L2
directory in a single step. This ought to be safe, since the M2P table can only
be extended, but never shrunk.

>+
>+    return EXCRET_fault_fixed;
>+}

You're leaking map_domain_page()-es here (and at the earlier return points).

(All the comments likewise apply to the subsequent 64-bit variant.)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel