[PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override

[PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override

[Ian Campbell]

The caller will not undo a mapping which failed and therefore the
override will not be removed.

This is especially bad in the case of GNTMAP_contains_pte mapping type
mappings where m2p_add_override will destroy the kernel mapping of the
page.

This was observed via a failure of map_grant_pages in gntdev_mmap (due
to userspace using a bad grant reference), which left the page in
question unmapped (because it was a GNTMAP_contains_pte mapping) which
led to a crash later on.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 drivers/xen/grant-table.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 1a9bc2b..d0e30ce 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -462,6 +462,10 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
 		return ret;
 
 	for (i = 0; i < count; i++) {
+		/* Do not add to override if the map failed. */
+		if (map_ops[i].status)
+			continue;
+
 		if (map_ops[i].flags & GNTMAP_contains_pte) {
 			pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) +
 				(map_ops[i].host_addr & ~PAGE_MASK));
-- 
1.5.6.5

Re: [PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override

[Ian Campbell]

On Fri, 2011-03-04 at 17:38 +0000, Ian Campbell wrote:
> The caller will not undo a mapping which failed and therefore the
> override will not be removed.
> 
> This is especially bad in the case of GNTMAP_contains_pte mapping type
> mappings where m2p_add_override will destroy the kernel mapping of the
> page.
> 
> This was observed via a failure of map_grant_pages in gntdev_mmap (due
> to userspace using a bad grant reference), which left the page in
> question unmapped (because it was a GNTMAP_contains_pte mapping) which
> led to a crash later on.

My original motivation for looking into this was a dom0 kernel crash in
gntdev exposed by using the qemu disk backend on 2.6.32 (by mistake due
to not loading blktap). The crash was in mn_release but was root caused
by a map_grant_pages() failing (due to a bogus grant ref).

It seems that Daniel's "xen-gntdev: Add reference counting to maps" +
"xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override" +
this fix are sufficient to fix it for 2.6.38-rc7+ but I wonder if we
also want it fixed for 2.6.32? I think in that case backporting Daniel's
patches is the best bet. (this fix won't be necessary, no m2p overlay in
2.6.32).

Ian.

> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> ---
>  drivers/xen/grant-table.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 1a9bc2b..d0e30ce 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -462,6 +462,10 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>  		return ret;
>  
>  	for (i = 0; i < count; i++) {
> +		/* Do not add to override if the map failed. */
> +		if (map_ops[i].status)
> +			continue;
> +
>  		if (map_ops[i].flags & GNTMAP_contains_pte) {
>  			pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) +
>  				(map_ops[i].host_addr & ~PAGE_MASK));

Re: [PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override

[Konrad Rzeszutek Wilk]

On Fri, Mar 04, 2011 at 05:47:08PM +0000, Ian Campbell wrote:
> On Fri, 2011-03-04 at 17:38 +0000, Ian Campbell wrote:
> > The caller will not undo a mapping which failed and therefore the
> > override will not be removed.
> > 
> > This is especially bad in the case of GNTMAP_contains_pte mapping type
> > mappings where m2p_add_override will destroy the kernel mapping of the
> > page.
> > 
> > This was observed via a failure of map_grant_pages in gntdev_mmap (due
> > to userspace using a bad grant reference), which left the page in
> > question unmapped (because it was a GNTMAP_contains_pte mapping) which
> > led to a crash later on.
> 
> My original motivation for looking into this was a dom0 kernel crash in
> gntdev exposed by using the qemu disk backend on 2.6.32 (by mistake due
> to not loading blktap). The crash was in mn_release but was root caused
> by a map_grant_pages() failing (due to a bogus grant ref).
> 
> It seems that Daniel's "xen-gntdev: Add reference counting to maps" +

This one:
http://article.gmane.org/gmane.comp.emulators.xen.devel/100076 ?

> "xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override" +
> this fix are sufficient to fix it for 2.6.38-rc7+ but I wonder if we

Can you point me to the "this fix" ? You mention three patches but
the last one is the same as 
"xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override"

so that looks to be two patches.

> also want it fixed for 2.6.32? I think in that case backporting Daniel's
> patches is the best bet. (this fix won't be necessary, no m2p overlay in
> 2.6.32).
> 
> Ian.
> 
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> > ---
> >  drivers/xen/grant-table.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> > index 1a9bc2b..d0e30ce 100644
> > --- a/drivers/xen/grant-table.c
> > +++ b/drivers/xen/grant-table.c
> > @@ -462,6 +462,10 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
> >  		return ret;
> >  
> >  	for (i = 0; i < count; i++) {
> > +		/* Do not add to override if the map failed. */
> > +		if (map_ops[i].status)
> > +			continue;
> > +
> >  		if (map_ops[i].flags & GNTMAP_contains_pte) {
> >  			pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) +
> >  				(map_ops[i].host_addr & ~PAGE_MASK));
> 

Re: [PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override

[Ian Campbell]

On Wed, 2011-03-09 at 16:44 +0000, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 04, 2011 at 05:47:08PM +0000, Ian Campbell wrote:
> > On Fri, 2011-03-04 at 17:38 +0000, Ian Campbell wrote:
> > > The caller will not undo a mapping which failed and therefore the
> > > override will not be removed.
> > > 
> > > This is especially bad in the case of GNTMAP_contains_pte mapping type
> > > mappings where m2p_add_override will destroy the kernel mapping of the
> > > page.
> > > 
> > > This was observed via a failure of map_grant_pages in gntdev_mmap (due
> > > to userspace using a bad grant reference), which left the page in
> > > question unmapped (because it was a GNTMAP_contains_pte mapping) which
> > > led to a crash later on.
> > 
> > My original motivation for looking into this was a dom0 kernel crash in
> > gntdev exposed by using the qemu disk backend on 2.6.32 (by mistake due
> > to not loading blktap). The crash was in mn_release but was root caused
> > by a map_grant_pages() failing (due to a bogus grant ref).
> > 
> > It seems that Daniel's "xen-gntdev: Add reference counting to maps" +
> 
> This one:
> http://article.gmane.org/gmane.comp.emulators.xen.devel/100076 ?

The one with title "Add reference counting to maps", it is
68b025c813c2eb41ff25628e3d4952d5185eb1a4 in your tree.

> > "xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override" +
> > this fix are sufficient to fix it for 2.6.38-rc7+ but I wonder if we
> 
> Can you point me to the "this fix" ?

The fix from the mail I was replying to e.g.
http://lists.xensource.com/archives/html/xen-devel/2011-03/msg00264.html

>  You mention three patches but
> the last one is the same as 
> "xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override"

I think I cut & pasted the wrong subject for one of them. The three
patches are:
        "xen-gntdev: Add reference counting to maps" (68b025c813c2e in your tree)
        "xen-gntdev: Fix memory leak when mmap fails" (90b6f30548a52 in your tree)
        "xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override" (AKA this fix)

I think the first two are sufficient for 2.6.32 since it doesn't have
the m2p override so it doesn't need the third one.

Ian.

> 
> so that looks to be two patches.
> 
> > also want it fixed for 2.6.32? I think in that case backporting Daniel's
> > patches is the best bet. (this fix won't be necessary, no m2p overlay in
> > 2.6.32).
> > 
> > Ian.
> > 
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> > > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> > > ---
> > >  drivers/xen/grant-table.c |    4 ++++
> > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> > > index 1a9bc2b..d0e30ce 100644
> > > --- a/drivers/xen/grant-table.c
> > > +++ b/drivers/xen/grant-table.c
> > > @@ -462,6 +462,10 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
> > >  		return ret;
> > >  
> > >  	for (i = 0; i < count; i++) {
> > > +		/* Do not add to override if the map failed. */
> > > +		if (map_ops[i].status)
> > > +			continue;
> > > +
> > >  		if (map_ops[i].flags & GNTMAP_contains_pte) {
> > >  			pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) +
> > >  				(map_ops[i].host_addr & ~PAGE_MASK));
> >