| Message ID | 20210119094122.23713-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/mm: Remove cascade damage from "fishy" ref/typecount failure | expand |
On 19/01/2021 09:41, Andrew Cooper wrote: > This code has been copied in 3 places, but it is broken and dangerous. > > For all these cases, the domain destruction path will underflow the whichever > reference failed to be taken, leading to all kinds of more fun bugs. > > Crashing instantly is strictly less-bad behaviour. > > 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> > > I'm pretty certain that underflowing the main refcount is a BUG() elsewhere. > I'm not certain what underflowing the typecount manages to do. Actually, they're both BUG() conditions. The typeref BUG() is in _put_page_type(), while the main refcount is caught in put_page_alloc_ref() for each of these examples. I'll reword the commit message, but the code change is good. ~Andrew
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 1cc27df87f..b2ceca7625 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -372,8 +372,7 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) * 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; + BUG(); } iorp->va = __map_domain_page_global(page); diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 2d4475ee3d..08f489d795 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3048,8 +3048,7 @@ static int vmx_alloc_vlapic_mapping(struct domain *d) * 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; + BUG(); } mfn = page_to_mfn(pg); diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c index 01281f786e..cfd91572b5 100644 --- a/xen/arch/x86/mm/mem_paging.c +++ b/xen/arch/x86/mm/mem_paging.c @@ -388,9 +388,7 @@ static int prepare(struct domain *d, gfn_t gfn, 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; + BUG(); } mfn = page_to_mfn(page); page_extant = 0;
This code has been copied in 3 places, but it is broken and dangerous. For all these cases, the domain destruction path will underflow the whichever reference failed to be taken, leading to all kinds of more fun bugs. Crashing instantly is strictly less-bad behaviour. 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> I'm pretty certain that underflowing the main refcount is a BUG() elsewhere. I'm not certain what underflowing the typecount manages to do. --- xen/arch/x86/hvm/ioreq.c | 3 +-- xen/arch/x86/hvm/vmx/vmx.c | 3 +-- xen/arch/x86/mm/mem_paging.c | 4 +--- 3 files changed, 3 insertions(+), 7 deletions(-)