Invalid P2M entries after gnttab unmap

Invalid P2M entries after gnttab unmap

[Daniel De Graaf]

When an HVM guest uses gnttab_map_grant_ref to map granted on top of valid
GFNs, it appears that the original MFNs referred to by these GFNs are lost.
The unmap operation sets the p2m mapping of the GFN to INVALID_MFN (and it
appears that replace_grant_p2m_mapping will not accept valid MFNs).

Most of the time, this appears to be true in testing. However, I have
noticed a few cases where the GFNs are valid following the unmap operation.
This has happened when a large number of grants (1284) are being unmapped
due to a domain shutdown; in this case, perhaps half of the unmapped GFNs
will point to valid memory, and half will point to invalid memory. In this
case, "invalid memory" discards writes and returns 0xFF on all reads; valid
memory appears to be normal RAM.

There doesn't appear to be any documentation on the intended behavior here.
>From the guest kernel's perspective, it makes the most sense for GFNs that
pointed to RAM prior to the map operation to continue to point to RAM after
the unmap operation - that is, the unmap fully undoes what the map did. The
contents of the pages (and which exact MFN they point to) aren't important.

-- 
Daniel De Graaf
National Security Agency

Re: Invalid P2M entries after gnttab unmap

[Tim Deegan]

Hi, 

At 16:34 +0000 on 04 Mar (1299256499), Daniel De Graaf wrote:
> When an HVM guest uses gnttab_map_grant_ref to map granted on top of valid
> GFNs, it appears that the original MFNs referred to by these GFNs are lost.

Yes.  The p2m table only holds one MFN for each PFN (and vice versa).
If you want to keep that memory you could move it somewhere else 
using XENMAPSPACE_gmfn, or just map your grant refs into an MMIO hole. 

> The unmap operation sets the p2m mapping of the GFN to INVALID_MFN.

There's nothing else it can set it to, really, if you take away the
thing that was there. 

> (and it appears that replace_grant_p2m_mapping will not accept valid MFNs).
> 
> Most of the time, this appears to be true in testing. However, I have
> noticed a few cases where the GFNs are valid following the unmap operation.

That seems like a bug. 

> This has happened when a large number of grants (1284) are being unmapped
> due to a domain shutdown;

Can you be more specific?  Do you mean that a domain that has mapped
grants into its p2m is shutting down in a controlled way, and is pulling
out all the grant mappings as it does so?  What hypercall does it use to
unmap the grants?

Cheers,

Tim.

> in this case, perhaps half of the unmapped GFNs
> will point to valid memory, and half will point to invalid memory. In this
> case, "invalid memory" discards writes and returns 0xFF on all reads; valid
> memory appears to be normal RAM.
> 
> There doesn't appear to be any documentation on the intended behavior here.
> >From the guest kernel's perspective, it makes the most sense for GFNs that
> pointed to RAM prior to the map operation to continue to point to RAM after
> the unmap operation - that is, the unmap fully undoes what the map did. The
> contents of the pages (and which exact MFN they point to) aren't important.
> 
> -- 
> Daniel De Graaf
> National Security Agency
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: Invalid P2M entries after gnttab unmap

[Ian Campbell]

On Fri, 2011-03-04 at 17:02 +0000, Tim Deegan wrote:
> Hi, 
> 
> At 16:34 +0000 on 04 Mar (1299256499), Daniel De Graaf wrote:
> > When an HVM guest uses gnttab_map_grant_ref to map granted on top of valid
> > GFNs, it appears that the original MFNs referred to by these GFNs are lost.
> 
> Yes.  The p2m table only holds one MFN for each PFN (and vice versa).
> If you want to keep that memory you could move it somewhere else 
> using XENMAPSPACE_gmfn,

In which case you might as well do the grant map to "somewhere else" I
guess?

>  or just map your grant refs into an MMIO hole. 

The platform-pci device has a BAR for this sort of purpose, doesn't it?
Mostly it just gets used for the grant table itself and perhaps it isn't
large enough to be a suitable source of mapping space.

Is there some reason the gntdev driver can't just balloon down
(XENMEM_decrease_reservation) to make itself a space to map and then
balloon back up (XENMEM_increase_reservation) after unmap when running
HVM?

> > in this case, perhaps half of the unmapped GFNs
> > will point to valid memory, and half will point to invalid memory. In this
> > case, "invalid memory" discards writes and returns 0xFF on all reads; valid
> > memory appears to be normal RAM.

The workaround relies entirely on this discard and read 0xff behaviour,
which I'm not sure is wise.

I'm not especially happy about the idea of 2.6.39 getting released into
the wild with this hack in it. Luckily there is plenty of time to fix
the issue properly before then.

Ian.

Re: Invalid P2M entries after gnttab unmap

[Daniel De Graaf]

On 03/04/2011 01:34 PM, Ian Campbell wrote:
> On Fri, 2011-03-04 at 17:02 +0000, Tim Deegan wrote:
>> Hi, 
>>
>> At 16:34 +0000 on 04 Mar (1299256499), Daniel De Graaf wrote:
>>> When an HVM guest uses gnttab_map_grant_ref to map granted on top of valid
>>> GFNs, it appears that the original MFNs referred to by these GFNs are lost.
>>
>> Yes.  The p2m table only holds one MFN for each PFN (and vice versa).
>> If you want to keep that memory you could move it somewhere else 
>> using XENMAPSPACE_gmfn,
> 
> In which case you might as well do the grant map to "somewhere else" I
> guess?

Yes, remapping seems to be useless unless there's a "somewhere else" that
isn't usable for normal memory access.

>>  or just map your grant refs into an MMIO hole. 
> 
> The platform-pci device has a BAR for this sort of purpose, doesn't it?
> Mostly it just gets used for the grant table itself and perhaps it isn't
> large enough to be a suitable source of mapping space.
> 
> Is there some reason the gntdev driver can't just balloon down
> (XENMEM_decrease_reservation) to make itself a space to map and then
> balloon back up (XENMEM_increase_reservation) after unmap when running
> HVM?

I recall having problems with doing this last time, but I think other changes
to make the balloon driver work in HVM may have fixed the issue. I'll try it
again; I think this would be the best solution.

It may be useful to integrate with the balloon driver and use some of the pages
that have already been ballooned down, rather than forcing the map to be a two-
step process. This would also make handling a failure to balloon back up far
easier. This would add a dependency on the balloon module and a new interface
that allows requesting/returning ballooned pages.

>>> in this case, perhaps half of the unmapped GFNs
>>> will point to valid memory, and half will point to invalid memory. In this
>>> case, "invalid memory" discards writes and returns 0xFF on all reads; valid
>>> memory appears to be normal RAM.
> 
> The workaround relies entirely on this discard and read 0xff behaviour,
> which I'm not sure is wise.
> 
> I'm not especially happy about the idea of 2.6.39 getting released into
> the wild with this hack in it. Luckily there is plenty of time to fix
> the issue properly before then.
> 
> Ian.
> 

Agreed, it would be best to avoid this workaround since the intended behavior
of unmapping is to produce invalid PFNs (I had written it assuming they were
intended to be valid). Using the ballooning hypercalls should fix this.

-- 
Daniel De Graaf
National Security Agency

[RFC/PATCH v1] xen-gntdev: Use XENMEM_populate_physmap on unmapped pages in HVM

[Daniel De Graaf]

>> Is there some reason the gntdev driver can't just balloon down
>> (XENMEM_decrease_reservation) to make itself a space to map and then
>> balloon back up (XENMEM_increase_reservation) after unmap when running
>> HVM?
> 
> I recall having problems with doing this last time, but I think other changes
> to make the balloon driver work in HVM may have fixed the issue. I'll try it
> again; I think this would be the best solution.

Using the balloon hypercalls does work in my test. Below is a patch that
uses them directly in gntdev, which is simpler but doesn't handle errors
in re-ballooning and doesn't take advantage of already-ballooned pages
from the balloon device. Taking advantage of the balloon device is also
possible and would be more efficient, but adds another dependency to
gntdev. Any opinions on which method is preferred? If not, I'll try
making a patch to implement the balloon-assisted version.

8<-------------------------------------------------------------------------

Grant mappings cause the PFN<->MFN mapping to be lost on the pages
used for the mapping. Instead of leaking memory, explicitly free it
to the hypervisor and request the memory back after unmapping. This
removes the need for the bad-page leak workaround.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntdev.c |   58 +++++++++++++++++++++++++++++++++----------------
 1 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index d43ff30..104098a 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -41,6 +41,7 @@
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/page.h>
+#include <xen/interface/memory.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Derek G. Murray <Derek.Murray@cl.cam.ac.uk>, "
@@ -203,27 +204,9 @@ static void gntdev_put_map(struct grant_map *map)
 			unmap_grant_pages(map, 0, map->count);
 
 		for (i = 0; i < map->count; i++) {
-			uint32_t check, *tmp;
 			if (!map->pages[i])
 				continue;
-			/* XXX When unmapping in an HVM domain, Xen will
-			 * sometimes end up mapping the GFN to an invalid MFN.
-			 * In this case, writes will be discarded and reads will
-			 * return all 0xFF bytes.  Leak these unusable GFNs
-			 * until Xen supports fixing their p2m mapping.
-			 *
-			 * Confirmed present in Xen 4.1-RC3 with HVM source
-			 */
-			tmp = kmap(map->pages[i]);
-			*tmp = 0xdeaddead;
-			mb();
-			check = *tmp;
-			kunmap(map->pages[i]);
-			if (check == 0xdeaddead)
-				__free_page(map->pages[i]);
-			else
-				pr_debug("Discard page %d=%ld\n", i,
-					page_to_pfn(map->pages[i]));
+			__free_page(map->pages[i]);
 		}
 	}
 	kfree(map->pages);
@@ -258,11 +241,19 @@ static int map_grant_pages(struct grant_map *map)
 {
 	int i, err = 0;
 	phys_addr_t addr;
+	unsigned long frame;
 
 	if (!use_ptemod) {
+		struct xen_memory_reservation reservation = {
+			.address_bits = 0,
+			.extent_order = 0,
+			.nr_extents   = 1,
+			.domid        = DOMID_SELF
+		};
 		/* Note: it could already be mapped */
 		if (map->map_ops[0].handle != -1)
 			return 0;
+		set_xen_guest_handle(reservation.extent_start, &frame);
 		for (i = 0; i < map->count; i++) {
 			addr = (phys_addr_t)
 				pfn_to_kaddr(page_to_pfn(map->pages[i]));
@@ -271,6 +262,12 @@ static int map_grant_pages(struct grant_map *map)
 				map->grants[i].domid);
 			gnttab_set_unmap_op(&map->unmap_ops[i], addr,
 				map->flags, -1 /* handle */);
+			/* TODO batch hypercall, needs buffer */
+			frame = page_to_pfn(map->pages[i]);
+			err = HYPERVISOR_memory_op(XENMEM_decrease_reservation,
+				&reservation);
+			if (WARN_ON(err <= 0))
+				printk(KERN_INFO "memop returned %d\n", err);
 		}
 	}
 
@@ -324,6 +321,29 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
 			map->unmap_ops[offset+i].status);
 		map->unmap_ops[offset+i].handle = -1;
 	}
+
+	if (!use_ptemod) {
+		struct xen_memory_reservation reservation = {
+			.address_bits = 0,
+			.extent_order = 0,
+			.nr_extents   = 1,
+			.domid        = DOMID_SELF
+		};
+		int rc;
+		unsigned long frame;
+		set_xen_guest_handle(reservation.extent_start, &frame);
+		/* TODO batch hypercall, needs buffer */
+		for (i = offset; i < pages + offset; i++) {
+			frame = page_to_pfn(map->pages[i]);
+			rc = HYPERVISOR_memory_op(XENMEM_populate_physmap,
+				&reservation);
+			if (WARN_ON(rc <= 0)) {
+				map->pages[i] = NULL;
+				continue;
+			}
+		}
+	}
+
 	return err;
 }
 
-- 
1.7.3.4

Re: [RFC/PATCH v1] xen-gntdev: Use XENMEM_populate_physmap on unmapped pages in HVM

[Ian Campbell]

On Fri, 2011-03-04 at 20:53 +0000, Daniel De Graaf wrote:
> >> Is there some reason the gntdev driver can't just balloon down
> >> (XENMEM_decrease_reservation) to make itself a space to map and then
> >> balloon back up (XENMEM_increase_reservation) after unmap when running
> >> HVM?
> > 
> > I recall having problems with doing this last time, but I think other changes
> > to make the balloon driver work in HVM may have fixed the issue. I'll try it
> > again; I think this would be the best solution.
> 
> Using the balloon hypercalls does work in my test. Below is a patch that
> uses them directly in gntdev, which is simpler but doesn't handle errors
> in re-ballooning and doesn't take advantage of already-ballooned pages
> from the balloon device. Taking advantage of the balloon device is also
> possible and would be more efficient, but adds another dependency to
> gntdev. Any opinions on which method is preferred? If not, I'll try
> making a patch to implement the balloon-assisted version.

The balloon driver in the 2.6.32 pvops (and all old-style Xen kernels
too) has interfaces which are used by the backend drivers for exactly
this purpose.

It was always a bit of a wart that the backends depended on the balloon
driver in this way. IMHO the core ballooning functionality and kernel
interfaces should be moved into the core kernel under CONFIG_XEN. All
that should remain under CONFIG_XEN_BALLOON is the sysfs and xenbus
integration e.g. the user/admin visible interfaces.

This is somewhat analogous to drivers/xen/{events,grant-table}.c being
core kernel and drivers/xen/{gnt,evt}dev.c being configurable optional
modules.

It's possible that what remains under CONFIG_XEN_BALLOON is such a tiny
amount of code that it's not worth keeping it as a separate option,
although the "depends SYSFS" aspect may make it more worthwhile.

Ian.

> 
> 8<-------------------------------------------------------------------------
> 
> Grant mappings cause the PFN<->MFN mapping to be lost on the pages
> used for the mapping. Instead of leaking memory, explicitly free it
> to the hypervisor and request the memory back after unmapping. This
> removes the need for the bad-page leak workaround.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/gntdev.c |   58 +++++++++++++++++++++++++++++++++----------------
>  1 files changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index d43ff30..104098a 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -41,6 +41,7 @@
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/page.h>
> +#include <xen/interface/memory.h>
>  
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Derek G. Murray <Derek.Murray@cl.cam.ac.uk>, "
> @@ -203,27 +204,9 @@ static void gntdev_put_map(struct grant_map *map)
>  			unmap_grant_pages(map, 0, map->count);
>  
>  		for (i = 0; i < map->count; i++) {
> -			uint32_t check, *tmp;
>  			if (!map->pages[i])
>  				continue;
> -			/* XXX When unmapping in an HVM domain, Xen will
> -			 * sometimes end up mapping the GFN to an invalid MFN.
> -			 * In this case, writes will be discarded and reads will
> -			 * return all 0xFF bytes.  Leak these unusable GFNs
> -			 * until Xen supports fixing their p2m mapping.
> -			 *
> -			 * Confirmed present in Xen 4.1-RC3 with HVM source
> -			 */
> -			tmp = kmap(map->pages[i]);
> -			*tmp = 0xdeaddead;
> -			mb();
> -			check = *tmp;
> -			kunmap(map->pages[i]);
> -			if (check == 0xdeaddead)
> -				__free_page(map->pages[i]);
> -			else
> -				pr_debug("Discard page %d=%ld\n", i,
> -					page_to_pfn(map->pages[i]));
> +			__free_page(map->pages[i]);
>  		}
>  	}
>  	kfree(map->pages);
> @@ -258,11 +241,19 @@ static int map_grant_pages(struct grant_map *map)
>  {
>  	int i, err = 0;
>  	phys_addr_t addr;
> +	unsigned long frame;
>  
>  	if (!use_ptemod) {
> +		struct xen_memory_reservation reservation = {
> +			.address_bits = 0,
> +			.extent_order = 0,
> +			.nr_extents   = 1,
> +			.domid        = DOMID_SELF
> +		};
>  		/* Note: it could already be mapped */
>  		if (map->map_ops[0].handle != -1)
>  			return 0;
> +		set_xen_guest_handle(reservation.extent_start, &frame);
>  		for (i = 0; i < map->count; i++) {
>  			addr = (phys_addr_t)
>  				pfn_to_kaddr(page_to_pfn(map->pages[i]));
> @@ -271,6 +262,12 @@ static int map_grant_pages(struct grant_map *map)
>  				map->grants[i].domid);
>  			gnttab_set_unmap_op(&map->unmap_ops[i], addr,
>  				map->flags, -1 /* handle */);
> +			/* TODO batch hypercall, needs buffer */
> +			frame = page_to_pfn(map->pages[i]);
> +			err = HYPERVISOR_memory_op(XENMEM_decrease_reservation,
> +				&reservation);
> +			if (WARN_ON(err <= 0))
> +				printk(KERN_INFO "memop returned %d\n", err);
>  		}
>  	}
>  
> @@ -324,6 +321,29 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
>  			map->unmap_ops[offset+i].status);
>  		map->unmap_ops[offset+i].handle = -1;
>  	}
> +
> +	if (!use_ptemod) {
> +		struct xen_memory_reservation reservation = {
> +			.address_bits = 0,
> +			.extent_order = 0,
> +			.nr_extents   = 1,
> +			.domid        = DOMID_SELF
> +		};
> +		int rc;
> +		unsigned long frame;
> +		set_xen_guest_handle(reservation.extent_start, &frame);
> +		/* TODO batch hypercall, needs buffer */
> +		for (i = offset; i < pages + offset; i++) {
> +			frame = page_to_pfn(map->pages[i]);
> +			rc = HYPERVISOR_memory_op(XENMEM_populate_physmap,
> +				&reservation);
> +			if (WARN_ON(rc <= 0)) {
> +				map->pages[i] = NULL;
> +				continue;
> +			}
> +		}
> +	}
> +
>  	return err;
>  }
>  

Re: Invalid P2M entries after gnttab unmap

[Daniel De Graaf]

On 03/04/2011 12:02 PM, Tim Deegan wrote:
> Hi, 
> 
> At 16:34 +0000 on 04 Mar (1299256499), Daniel De Graaf wrote:
>> When an HVM guest uses gnttab_map_grant_ref to map granted on top of valid
>> GFNs, it appears that the original MFNs referred to by these GFNs are lost.
> 
> Yes.  The p2m table only holds one MFN for each PFN (and vice versa).
> If you want to keep that memory you could move it somewhere else 
> using XENMAPSPACE_gmfn, or just map your grant refs into an MMIO hole. 
> 

[addressed in reply to Ian's mail]

>> The unmap operation sets the p2m mapping of the GFN to INVALID_MFN.
> 
> There's nothing else it can set it to, really, if you take away the
> thing that was there. 
>
>> (and it appears that replace_grant_p2m_mapping will not accept valid MFNs).
>>
>> Most of the time, this appears to be true in testing. However, I have
>> noticed a few cases where the GFNs are valid following the unmap operation.
> 
> That seems like a bug. 
> 
>> This has happened when a large number of grants (1284) are being unmapped
>> due to a domain shutdown;
> 
> Can you be more specific?  Do you mean that a domain that has mapped
> grants into its p2m is shutting down in a controlled way, and is pulling
> out all the grant mappings as it does so?  What hypercall does it use to
> unmap the grants?

I produced this in the following test scenario:

Domain 1 (HVM) is a display server that uses gntdev to map the vfb device from
domain 2 (also HVM, using fbfront). Domain 2 was shut down using xm destroy,
which triggered (via xenstore) domain 1 to unmap all of the grants. I think I
also observed this with a normal shutdown of domain 2.

> Cheers,
> 
> Tim.
> 
>> in this case, perhaps half of the unmapped GFNs
>> will point to valid memory, and half will point to invalid memory. In this
>> case, "invalid memory" discards writes and returns 0xFF on all reads; valid
>> memory appears to be normal RAM.
>>
>> There doesn't appear to be any documentation on the intended behavior here.
>> >From the guest kernel's perspective, it makes the most sense for GFNs that
>> pointed to RAM prior to the map operation to continue to point to RAM after
>> the unmap operation - that is, the unmap fully undoes what the map did. The
>> contents of the pages (and which exact MFN they point to) aren't important.
>>
>> -- 
>> Daniel De Graaf
>> National Security Agency
>>