Missing checks for xc_map_foreign_range() failure

Other, more naive uses would profit from a less awkward convenience function that either succeeds completely or fails completely. As far as I can see, these uses all neglect to check for the second failure mode so far. Potential crash bugs.

Quoted 4 lines · Markus Armbruster

Other, more naive uses would profit from a less awkward convenience function that either succeeds completely or fails completely. As far as I can see, these uses all neglect to check for the second failure mode so far. Potential crash bugs.

Yes, that makes sense. Are you proposing to make a patch?

Quoted 10 lines · Keir Fraser

Quoted 5 lines · Markus Armbruster

Other, more naive uses would profit from a less awkward convenience function that either succeeds completely or fails completely. As far as I can see, these uses all neglect to check for the second failure mode so far. Potential crash bugs.

Yes, that makes sense. Are you proposing to make a patch?

Happy to do that :)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

hi
what is the function of the xc_map_foreign_range(), what does it work for ? could you give me someexplanation or example about it , i encounter it in the code of xc_domain_save(), and confused about the function of it,could you help me

Thanks in advance

From: Markus Armbruster
Sent: 2007年9月6日 0:37

xc_map_foreign_batch() can succeed partially. It is awkward to use when you're only interested in complete success. Provide new xc_map_foreign_pages() convenience function for that kind of use. Also convert two obvious calls to use it.

Signed-off-by Markus Armbruster <[email protected]>

---
The patch converts only those uses of xc_map_foreign_batch() to xc_map_foreign_pages() where I'm 100% sure it's safe. For the others, I'd rather have a second opinion from somebody familiar with the code.

Discussion of uses:

* qemu_remap_bucket() in tools/ioemu/vl.c

Tests for mapping failure, although in a slightly sloppy way:

     !(pfns[i + --j] & 0xF0000000UL)

This is true when any of the four bits is set. But mapping failed only when all of them are set, so maybe it should do this instead:

     (pfns[i + --j] & 0xF0000000UL) != 0xF0000000UL

* main() in tools/ioemu/vl.c, in __ia64__ code

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages().

* set_vram_mapping() in tools/ioemu/hw/cirrus_vga.c

Doesn't test for mapping failure. Patch converts this one to xc_map_foreign_pages().

* copy_from_GFW_to_nvram() in tools/libxc/ia64/xc_ia64_hvm_build.c

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages(), but I have no way to test it.

* xc_core_arch_map_p2m() in tools/libxc/xc_core_x86.c

This maps a two level page directory: first a a page of pfns for the page directory (live_p2m_frame_list_list), using that pages of pfns for the pages themselves (live_p2m_frame_list), and using that the actual pages.

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages().

* xc_domain_restore() in tools/libxc/xc_domain_restore.c

Three uses, none of them tests for mapping failure.

I don't understand this code.

The first use of xc_map_foreign_batch() appears to deliberately pass invalid PFNs.

* map_and_save_p2m_table() in tools/libxc/xc_domain_save.c

Similar to xc_core_arch_map_p2m().

* xc_domain_save() in tools/libxc/xc_domain_save.c

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages().

* xc_domain_resume_any() in tools/libxc/xc_resume.c

Similar to xc_core_arch_map_p2m().

* xenfb_map_fb() in tools/xenfb/xenfb.c

Another map through a two level page directory, roughly similar to xc_core_arch_map_p2m().

Doesn't test for mapping failure. Patch converts this one to xc_map_foreign_pages().

diff -r 256160ff19b7 tools/ioemu/hw/cirrus_vga.c

tools/ioemu/hw/cirrus_vga.c +1 -1

@@ -2561,7 +2561,7 @@ static void *set_vram_mapping(unsigned l
 
    set_mm_mapping(xc_handle, domid, nr_extents, 0, extent_start);
 
-    vram_pointer = xc_map_foreign_batch(xc_handle, domid,
+    vram_pointer = xc_map_foreign_pages(xc_handle, domid,
 
 PROT_READ|PROT_WRITE,
                                        extent_start, nr_extents);

    if (vram_pointer == NULL) {

diff -r 256160ff19b7 tools/libxc/xc_misc.c

tools/libxc/xc_misc.c +21 -0

@@ -224,6 +224,39 @@ int xc_hvm_set_pci_link_route(
    unlock_pages(&arg, sizeof(arg));
 
    return rc;
+}
+
+void *xc_map_foreign_pages(int xc_handle, uint32_t dom, int prot,
+                           const xen_pfn_t *arr, int num)
+{
+    xen_pfn_t *pfn;
+    void *res;
+    int i;
+
+    pfn = malloc(num * sizeof(*pfn));
+    if (!pfn)
+        return NULL;
+    memcpy(pfn, arr, num * sizeof(*pfn));
+
+    res = xc_map_foreign_batch(xc_handle, dom, prot, pfn, num);
+    if (res) {
+        for (i = 0; i < num; i++) {
+            if ((pfn[i] & 0xF0000000UL) == 0xF0000000UL) {
+                /*
+                 * xc_map_foreign_batch() doesn't give us an error
+                 * code, so we have to make one up.  May not be

the

tools/libxc/xc_misc.c +12 -0

+                 * appropriate one.
+                 */
+                errno = EINVAL;
+                munmap(res, num * PAGE_SIZE);
+                res = NULL;
+                break;
+            }
+        }
+    }
+
+    free(pfn);
+    return res;

}

/*
diff -r 256160ff19b7 tools/libxc/xenctrl.h

tools/libxc/xenctrl.h +8 -0

@@ -646,6 +646,14 @@ void *xc_map_foreign_range(int xc_handle
                            int size, int prot,
                            unsigned long mfn );
 
+void *xc_map_foreign_pages(int xc_handle, uint32_t dom, int prot,
+                           const xen_pfn_t *arr, int num );
+
+/**
+ * Like xc_map_foreign_pages(), except it can succeeed partially.
+ * When a page cannot be mapped, its PFN in @arr is or'ed with
+ * 0xF0000000 to indicate the error.
+ */
 void *xc_map_foreign_batch(int xc_handle, uint32_t dom, int prot,
                           xen_pfn_t *arr, int num );
 

diff -r 256160ff19b7 tools/xenfb/xenfb.c

tools/xenfb/xenfb.c +2 -8

@@ -398,21 +398,15 @@ static int xenfb_map_fb(struct xenfb_pri
 	if (!pgmfns || !fbmfns)
 		goto out;
 
-	/*
-	 * Bug alert: xc_map_foreign_batch() can fail partly and
-	 * return a non-null value.  This is a design flaw.  When it
-	 * happens, we happily continue here, and later crash on
-	 * access.
-	 */
 	xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
-	map = xc_map_foreign_batch(xenfb->xc, domid,
+	map = xc_map_foreign_pages(xenfb->xc, domid,
 				   PROT_READ, pgmfns, n_fbdirs);
 	if (map == NULL)
 		goto out;
 	xenfb_copy_mfns(mode, n_fbmfns, fbmfns, map);
 	munmap(map, n_fbdirs * XC_PAGE_SIZE);
 
-	xenfb->pub.pixels = xc_map_foreign_batch(xenfb->xc, domid,
+	xenfb->pub.pixels = xc_map_foreign_pages(xenfb->xc, domid,
 				PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
 	if (xenfb->pub.pixels == NULL)
 		goto out;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Just curious why this can't be fixed in kernel driver? Is there case that caller is tolerable with partial mapping?

From: Keir Fraser [mailto:[email protected]]
Sent: 2007年9月6日 16:47

Quoted 2 lines · Kevin

Just curious why this can't be fixed in kernel driver? Is there case that caller is tolerable with partial mapping?

We would need an extra command in the kernel driver, as the current partial-success semantics are used appropriately by some callers. Doing it
in libxc avoids requiring a kernel upgrade to fix this issue.

-- Keir

Quoted 13 lines

From: Keir Fraser [mailto:[email protected]]
Sent: 2007年9月6日 16:47

Quoted 2 lines · Kevin

Just curious why this can't be fixed in kernel driver? Is there case that caller is tolerable with partial mapping?

We would need an extra command in the kernel driver, as the current partial-success semantics are used appropriately by some callers. Doing it
in libxc avoids requiring a kernel upgrade to fix this issue.

Quoted 1 line

-- Keir

Interesting... any example I can learn? Yes, compatibility is one concern.

Thanks,
Kevin

Also why do you need an extra allocation and copy to arr?

The patch converts only those uses of xc_map_foreign_batch() to xc_map_foreign_pages() where I'm 100% sure it's safe. For the others, I'd rather have a second opinion from somebody familiar with the code.

Discussion of uses:

* main() in tools/ioemu/vl.c, in __ia64__ code

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages().

* copy_from_GFW_to_nvram() in tools/libxc/ia64/xc_ia64_hvm_build.c

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages(), but I have no way to test it.

* xc_core_arch_map_p2m() in tools/libxc/xc_core_x86.c

This maps a two level page directory: first a a page of pfns for the page directory (live_p2m_frame_list_list), using that pages of pfns for the pages themselves (live_p2m_frame_list), and using that the actual pages.

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages().

Quoted 5 lines · Markus Armbruster

The patch converts only those uses of xc_map_foreign_batch() to xc_map_foreign_pages() where I'm 100% sure it's safe. For the others, I'd rather have a second opinion from somebody familiar with the code.

Discussion of uses:

...

Quoted 4 lines

* main() in tools/ioemu/vl.c, in __ia64__ code

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages().

This can't be replaced because some pages, e.g. VGA area [0xA0000, 0xC0000), can't be mapped.

Quoted 4 lines

* copy_from_GFW_to_nvram() in tools/libxc/ia64/xc_ia64_hvm_build.c

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages(), but I have no way to test it.

This can be replaced with xc_map_foreign_pages()
This change will go through xen-ia64-devel.hg.
I'll send the patch to xen-ia64-devel.

Quoted 9 lines

* xc_core_arch_map_p2m() in tools/libxc/xc_core_x86.c

This maps a two level page directory: first a a page of pfns for the page directory (live_p2m_frame_list_list), using that pages of pfns for the pages themselves (live_p2m_frame_list), and using that the actual pages.

Doesn't test for mapping failure. Likely candidate for xc_map_foreign_pages().

This can be replaced with xc_map_foreign_pages().