Ian Campbell
2011-Mar-04 17:38 UTC
[Xen-devel] [PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-04 17:47 UTC
[Xen-devel] Re: [PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override
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));_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Mar-09 16:44 UTC
[Xen-devel] Re: [PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override
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 weCan 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)); >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-09 16:54 UTC
[Xen-devel] Re: [PATCH] xen/p2m/m2p/gnttab: do not add failed grant maps to m2p override
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)); > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel