diff mbox series

x86/mm: Remove cascade damage from "fishy" ref/typecount failure

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

Commit Message

Andrew Cooper Jan. 19, 2021, 9:41 a.m. UTC
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(-)

Comments

Andrew Cooper Jan. 19, 2021, 11:34 a.m. UTC | #1
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 mbox series

Patch

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;