Re: [PATCH v3] x86/mm: Short circuit damage from "fishy" ref/typecount failure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.01.2021 18:59, Andrew Cooper wrote:
> I am literally not changing the current behaviour.  Xen *will* hit a
> BUG() if any of these domain_crash() paths are taken.
> 
> If you do not believe me, then please go and actually check what happens
> when simulating a ref-acquisition failure.

Okay, I've started with the debugging patch below. After the
other error handling fixed (see the patch just sent), this
works fine and - with the changes not marked "//temp" - even
stops leaking the page in that case. This latter fact proves
(to me) that at least on this path there's no ref lost
anywhere, and there's also no BUG() elsewhere that we would
trigger. As re-assurance I observed subsequent run attempts
of the same guest to end up re-using this same page for this
same purpose in a number of cases.

You patch altered two other, similar paths, and I can't
exclude there to be a problem there. Since the exercise was
useful for fixing the other two bugs anyway, I guess I'll do
the same for those paths later on and see what I get.

Jan

--- unstable.orig/xen/arch/x86/hvm/vmx/vmx.c
+++ unstable/xen/arch/x86/hvm/vmx/vmx.c
@@ -3076,13 +3076,22 @@ static int vmx_alloc_vlapic_mapping(stru
     if ( !pg )
         return -ENOMEM;
 
-    if ( !get_page_and_type(pg, d, PGT_writable_page) )
+printk("%pd: APIC access MFN: %lx (c=%lx t=%lx)\n", d, mfn_x(page_to_mfn(pg)), 
pg->count_info, pg->u.inuse.type_info);//temp
+//temp    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);
+        if ( get_page(pg, d) )
+        {
+            put_page_alloc_ref(pg);
+printk("%pd: MFN %lx: (c=%lx t=%lx)\n", d, mfn_x(page_to_mfn(pg)), 
pg->count_info, pg->u.inuse.type_info);//temp
+            put_page(pg);
+        }
+        else
+            printk("%pd: leaking MFN %lx\n", d, mfn_x(page_to_mfn(pg)));
         return -ENODATA;
     }