Olaf Hering
2010-Dec-16 17:59 UTC
[Xen-devel] [PATCH] do_memory_op: cleanup if copy_to_guest fails
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;
+ }
}
}
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2010-Dec-16 18:32 UTC
Re: [Xen-devel] [PATCH] do_memory_op: cleanup if copy_to_guest fails
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Olaf Hering
2010-Dec-16 19:18 UTC
Re: [Xen-devel] [PATCH] do_memory_op: cleanup if copy_to_guest fails
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel