| Message ID | 20210119122756.27772-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure | expand |
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 19 January 2021 12:28 > To: Xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Roger Pau Monné > <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Tamas K Lengyel > <tamas@tklengyel.com> > Subject: [PATCH v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure > > This code has been copied in 3 places, but it is problematic. > > All cases will hit a BUG() later in domain teardown, when a the missing > type/count reference is underflowed. > > Don't complicated the logic by leaving a totally unqualified domain crash, and > a timebomb which will be triggered by the toolstack at a slightly later, and > seemingly unrelated, point. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Paul Durrant <paul@xen.org> > CC: Tamas K Lengyel <tamas@tklengyel.com> > > v2: > * Reword the commit message. > * Switch BUG() to BUG_ON() to further reduce code volume. > --- > xen/arch/x86/hvm/ioreq.c | 11 ++--------- > xen/arch/x86/hvm/vmx/vmx.c | 11 ++--------- > xen/arch/x86/mm/mem_paging.c | 17 ++++------------- > 3 files changed, 8 insertions(+), 31 deletions(-) > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > index 1cc27df87f..0c38cfa151 100644 > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -366,15 +366,8 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) > if ( !page ) > return -ENOMEM; > > - if ( !get_page_and_type(page, s->target, PGT_writable_page) ) > - { > - /* > - * The domain can't possibly know about this page yet, so failure > - * here is a clear indication of something fishy going on. > - */ > - domain_crash(s->emulator); > - return -ENODATA; > - } > + /* Domain can't know about this page yet - something fishy going on. */ > + BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page)); > > iorp->va = __map_domain_page_global(page); > if ( !iorp->va ) > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 2d4475ee3d..8e438cb781 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3042,15 +3042,8 @@ static int vmx_alloc_vlapic_mapping(struct domain *d) > if ( !pg ) > return -ENOMEM; > > - if ( !get_page_and_type(pg, d, PGT_writable_page) ) > - { > - /* > - * The domain can't possibly know about this page yet, so failure > - * here is a clear indication of something fishy going on. > - */ > - domain_crash(d); > - return -ENODATA; > - } > + /* Domain can't know about this page yet - something fishy going on. */ > + BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page)); Does this compile? s/page/pg s/s->target/d ...and similar below is needed AFAICT. Paul > > mfn = page_to_mfn(pg); > clear_domain_page(mfn); > diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c > index 01281f786e..6e90019e76 100644 > --- a/xen/arch/x86/mm/mem_paging.c > +++ b/xen/arch/x86/mm/mem_paging.c > @@ -379,19 +379,10 @@ static int prepare(struct domain *d, gfn_t gfn, > page = alloc_domheap_page(d, 0); > if ( unlikely(page == NULL) ) > goto out; > - if ( unlikely(!get_page(page, d)) ) > - { > - /* > - * The domain can't possibly know about this page yet, so failure > - * here is a clear indication of something fishy going on. > - */ > - gprintk(XENLOG_ERR, > - "%pd: fresh page for GFN %"PRI_gfn" in unexpected state\n", > - d, gfn_x(gfn)); > - domain_crash(d); > - page = NULL; > - goto out; > - } > + > + /* Domain can't know about this page yet - something fishy going on. */ > + BUG_ON(!get_page(page, s->target)); > + > mfn = page_to_mfn(page); > page_extant = 0; > > -- > 2.11.0
On 19/01/2021 12:45, Paul Durrant wrote: >> -----Original Message----- >> From: Andrew Cooper <andrew.cooper3@citrix.com> >> Sent: 19 January 2021 12:28 >> To: Xen-devel <xen-devel@lists.xenproject.org> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Roger Pau Monné >> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Tamas K Lengyel >> <tamas@tklengyel.com> >> Subject: [PATCH v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure >> >> This code has been copied in 3 places, but it is problematic. >> >> All cases will hit a BUG() later in domain teardown, when a the missing >> type/count reference is underflowed. >> >> Don't complicated the logic by leaving a totally unqualified domain crash, and >> a timebomb which will be triggered by the toolstack at a slightly later, and >> seemingly unrelated, point. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> CC: Paul Durrant <paul@xen.org> >> CC: Tamas K Lengyel <tamas@tklengyel.com> >> >> v2: >> * Reword the commit message. >> * Switch BUG() to BUG_ON() to further reduce code volume. >> --- >> xen/arch/x86/hvm/ioreq.c | 11 ++--------- >> xen/arch/x86/hvm/vmx/vmx.c | 11 ++--------- >> xen/arch/x86/mm/mem_paging.c | 17 ++++------------- >> 3 files changed, 8 insertions(+), 31 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c >> index 1cc27df87f..0c38cfa151 100644 >> --- a/xen/arch/x86/hvm/ioreq.c >> +++ b/xen/arch/x86/hvm/ioreq.c >> @@ -366,15 +366,8 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) >> if ( !page ) >> return -ENOMEM; >> >> - if ( !get_page_and_type(page, s->target, PGT_writable_page) ) >> - { >> - /* >> - * The domain can't possibly know about this page yet, so failure >> - * here is a clear indication of something fishy going on. >> - */ >> - domain_crash(s->emulator); >> - return -ENODATA; >> - } >> + /* Domain can't know about this page yet - something fishy going on. */ >> + BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page)); >> >> iorp->va = __map_domain_page_global(page); >> if ( !iorp->va ) >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >> index 2d4475ee3d..8e438cb781 100644 >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -3042,15 +3042,8 @@ static int vmx_alloc_vlapic_mapping(struct domain *d) >> if ( !pg ) >> return -ENOMEM; >> >> - if ( !get_page_and_type(pg, d, PGT_writable_page) ) >> - { >> - /* >> - * The domain can't possibly know about this page yet, so failure >> - * here is a clear indication of something fishy going on. >> - */ >> - domain_crash(d); >> - return -ENODATA; >> - } >> + /* Domain can't know about this page yet - something fishy going on. */ >> + BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page)); > Does this compile? > > s/page/pg > s/s->target/d > > ...and similar below is needed AFAICT. Urgh no - missing the delta in my working tree. Fixed both. ~Andrew
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 1cc27df87f..0c38cfa151 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -366,15 +366,8 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) if ( !page ) return -ENOMEM; - if ( !get_page_and_type(page, s->target, PGT_writable_page) ) - { - /* - * The domain can't possibly know about this page yet, so failure - * here is a clear indication of something fishy going on. - */ - domain_crash(s->emulator); - return -ENODATA; - } + /* Domain can't know about this page yet - something fishy going on. */ + BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page)); iorp->va = __map_domain_page_global(page); if ( !iorp->va ) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 2d4475ee3d..8e438cb781 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3042,15 +3042,8 @@ static int vmx_alloc_vlapic_mapping(struct domain *d) if ( !pg ) return -ENOMEM; - if ( !get_page_and_type(pg, d, PGT_writable_page) ) - { - /* - * The domain can't possibly know about this page yet, so failure - * here is a clear indication of something fishy going on. - */ - domain_crash(d); - return -ENODATA; - } + /* Domain can't know about this page yet - something fishy going on. */ + BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page)); mfn = page_to_mfn(pg); clear_domain_page(mfn); diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c index 01281f786e..6e90019e76 100644 --- a/xen/arch/x86/mm/mem_paging.c +++ b/xen/arch/x86/mm/mem_paging.c @@ -379,19 +379,10 @@ static int prepare(struct domain *d, gfn_t gfn, page = alloc_domheap_page(d, 0); if ( unlikely(page == NULL) ) goto out; - if ( unlikely(!get_page(page, d)) ) - { - /* - * The domain can't possibly know about this page yet, so failure - * here is a clear indication of something fishy going on. - */ - gprintk(XENLOG_ERR, - "%pd: fresh page for GFN %"PRI_gfn" in unexpected state\n", - d, gfn_x(gfn)); - domain_crash(d); - page = NULL; - goto out; - } + + /* Domain can't know about this page yet - something fishy going on. */ + BUG_ON(!get_page(page, s->target)); + mfn = page_to_mfn(page); page_extant = 0;
This code has been copied in 3 places, but it is problematic. All cases will hit a BUG() later in domain teardown, when a the missing type/count reference is underflowed. Don't complicated the logic by leaving a totally unqualified domain crash, and a timebomb which will be triggered by the toolstack at a slightly later, and seemingly unrelated, point. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Paul Durrant <paul@xen.org> CC: Tamas K Lengyel <tamas@tklengyel.com> v2: * Reword the commit message. * Switch BUG() to BUG_ON() to further reduce code volume. --- xen/arch/x86/hvm/ioreq.c | 11 ++--------- xen/arch/x86/hvm/vmx/vmx.c | 11 ++--------- xen/arch/x86/mm/mem_paging.c | 17 ++++------------- 3 files changed, 8 insertions(+), 31 deletions(-)