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:
> On 20/01/2021 08:06, Jan Beulich wrote:
>> Also, as far as "impossible" here goes - the constructs all
>> anyway exist only to deal with what we consider impossible.
>> The question therefore really is of almost exclusively
>> theoretical nature, and hence something like a counter
>> possibly overflowing imo needs to be accounted for as
>> theoretically possible, albeit impossible with today's
>> computers and realistic timing assumptions. If a counter
>> overflow occurred, it definitely wouldn't be because of a
>> bug in Xen, but because of abnormal behavior elsewhere.
>> Hence I remain unconvinced it is appropriate to deal with
>> the situation by BUG().
> 
> I'm not sure how to be any clearer.
> 
> 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.

So I've now also played the same game on the ioreq path (see
debugging patch below, and again with some non-"//temp"
changes actually improving overall behavior in that "impossible"
case). No BUG()s hit, no leaks (thanks to the extra changes),
no other anomalies observed.

Hence I'm afraid it is now really up to you to point out the
specific BUG()s (and additional context as necessary) that you
either believe could be hit, or that you have observed being hit.

Jan

--- unstable.orig/xen/arch/x86/hvm/ioreq.c
+++ unstable/xen/arch/x86/hvm/ioreq.c
@@ -366,13 +366,23 @@ static int hvm_alloc_ioreq_mfn(struct hv
     if ( !page )
         return -ENOMEM;
 
-    if ( !get_page_and_type(page, s->target, PGT_writable_page) )
+printk("%pd: %sioreq MFN: %lx (c=%lx t=%lx)\n", s->target, buf ? "buf" : "", 
mfn_x(page_to_mfn(page)), page->count_info, page->u.inuse.type_info);//temp
+//temp    if ( !get_page_and_type(page, s->target, PGT_writable_page) )
+if((buf == (s->target->domain_id & 1)) || !get_page_and_type(page, s->target, 
PGT_writable_page))//temp
     {
         /*
          * 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);
+        domain_crash(is_control_domain(s->emulator) ? s->target : s->emulator);
+        if ( get_page(page, s->target) )
+        {
+            put_page_alloc_ref(page);
+printk("%pd: MFN %lx: (c=%lx t=%lx)\n", s->target, mfn_x(page_to_mfn(page)), 
page->count_info, page->u.inuse.type_info);//temp
+            put_page(page);
+        }
+        else
+            printk("%pd: leaking %pd MFN %lx\n", s->emulator, s->target, 
mfn_x(page_to_mfn(page)));
         return -ENODATA;
     }
 
@@ -385,6 +395,7 @@ static int hvm_alloc_ioreq_mfn(struct hv
     return 0;
 
  fail:
+printk("%pd: %sioreq mfn: %lx (c=%lx t=%lx)\n", s->target, buf ? "buf" : "", 
mfn_x(page_to_mfn(page)), page->count_info, page->u.inuse.type_info);//temp
     put_page_alloc_ref(page);
     put_page_and_type(page);
 
@@ -404,6 +415,7 @@ static void hvm_free_ioreq_mfn(struct hv
     unmap_domain_page_global(iorp->va);
     iorp->va = NULL;
 
+printk("%pd: %sioreq mfn: %lx [c=%lx t=%lx]\n", s->target, buf ? "buf" : "", 
mfn_x(page_to_mfn(page)), page->count_info, page->u.inuse.type_info);//temp
     put_page_alloc_ref(page);
     put_page_and_type(page);
 }