[PATCH] do_memory_op: cleanup if copy_to_guest fails

[PATCH] do_memory_op: cleanup if copy_to_guest fails

[Olaf Hering]

Undo the page allocation in the ulikely event the copy_to_guest fails.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

---

I have not exercised this code path, it was found during code inspection in 4.0

 xen/common/memory.c |    9 +++++++++
 1 file changed, 9 insertions(+)

--- xen-unstable.hg-4.1.22548.orig/xen/common/memory.c
+++ xen-unstable.hg-4.1.22548/xen/common/memory.c
@@ -82,7 +82,10 @@ static void increase_reservation(struct
         {
             mfn = page_to_mfn(page);
             if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) )
+            {
+                free_domheap_pages(page, a->extent_order);
                 goto out;
+            }
         }
     }
 
@@ -144,7 +147,13 @@ static void populate_physmap(struct memo
 
                 /* Inform the domain of the new page's machine address. */ 
                 if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) )
+                {
+                    for ( j = 0; j < (1 << a->extent_order); j++ )
+                        set_gpfn_from_mfn(mfn + j, INVALID_M2P_ENTRY);
+                    guest_physmap_remove_page(d, gpfn, mfn, a->extent_order);
+                    free_domheap_pages(page, a->extent_order);
                     goto out;
+                }
             }
         }
     }

Re: [PATCH] do_memory_op: cleanup if copy_to_guest fails

[Keir Fraser]

On 16/12/2010 17:59, "Olaf Hering" <olaf@aepfle.de> wrote:

> Undo the page allocation in the ulikely event the copy_to_guest fails.

You can't really clean up in this case. Once a page has been alloc'ed to a
domain, it can immediately see it and map it. Trying to then
free_domheap_page() ignoring the current page reference count is actually
introducing a bug.

Leaving a bit of a mess on failed copy_to_guest is okay imo, as that is
usually a pretty fatal sign anyway. If there were good reason for cleaning
up better, we should at least be doing if (test_and_clear(PGC_allocated))
put_page().

 -- Keir

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
> 
> I have not exercised this code path, it was found during code inspection in
> 4.0
> 
>  xen/common/memory.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> --- xen-unstable.hg-4.1.22548.orig/xen/common/memory.c
> +++ xen-unstable.hg-4.1.22548/xen/common/memory.c
> @@ -82,7 +82,10 @@ static void increase_reservation(struct
>          {
>              mfn = page_to_mfn(page);
>              if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1))
> )
> +            {
> +                free_domheap_pages(page, a->extent_order);
>                  goto out;
> +            }
>          }
>      }
>  
> @@ -144,7 +147,13 @@ static void populate_physmap(struct memo
>  
>                  /* Inform the domain of the new page's machine address. */
>                  if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn,
> 1)) )
> +                {
> +                    for ( j = 0; j < (1 << a->extent_order); j++ )
> +                        set_gpfn_from_mfn(mfn + j, INVALID_M2P_ENTRY);
> +                    guest_physmap_remove_page(d, gpfn, mfn, a->extent_order);
> +                    free_domheap_pages(page, a->extent_order);
>                      goto out;
> +                }
>              }
>          }
>      }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] do_memory_op: cleanup if copy_to_guest fails

[Olaf Hering]

On Thu, Dec 16, Keir Fraser wrote:

> Leaving a bit of a mess on failed copy_to_guest is okay imo, as that is
> usually a pretty fatal sign anyway. If there were good reason for cleaning
> up better, we should at least be doing if (test_and_clear(PGC_allocated))
> put_page().

Yes, it would leak just a page and from what I have seen, the Linux
drivers will BUG() most of the time anyway if the hypercall fails.
So I will drop this from my series.

Olaf