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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.01.2021 19:09, Andrew Cooper wrote:
> On 19/01/2021 16:48, Jan Beulich wrote:
>> On 19.01.2021 14:02, Andrew Cooper wrote:
>>> 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.
>> I'm afraid I could use some help with this: Why would there
>> be a missing reference, when the getting of one failed?
> 
> Look at the cleanup logic for the associated fields.
> 
> Either the plain ref fails (impossible without other fatal refcounting
> errors AFAICT), or the typeref fails (a concern, but impossible AFAICT).

In principle I would agree, if there wasn't the question of
count overflows. The type count presently is 56 bits wide,
while the general refcount has 54 bits. It'll be a long time
until they overflow, but it's not impossible. The underlying
problem there that I see is - where do we draw the line
between "can't possibly overflow in practice" (as we would
typically assume for 64-bit counters) and "is to be expected
to overflow (as we would typically assume for 32-bit
counters)?

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().

But yes, if otoh we assume the failure here to be the result
of a bug elsewhere in Xen (and not an overflow), then BUG()
may be warranted. Yet afaic these constructs weren't meant
to deal with bugs elsewhere in Xen, but with the
"impossible". So if we change our collective mind here, I
think the conversion to BUG() would then need accompanying
by respective commentary.

> When the plain ref fails, put_page_alloc_ref() spots the underflow with
> a BUG, while if the typeref fails, it is _put_page_type()'s BUG which
> spots the underflow.

put_page_alloc_ref() puts the ref installed by assign_pages()
aiui. If that one underflows, a bad put must have been invoked
elsewhere, which then is the one to fix. _put_page_type()
spotting an underflow also means either that very invocation
shouldn't have happened, or an earlier one was issued in
error.

At the example of hvm_alloc_ioreq_mfn(), in case of hitting
the path in question the only ref to the page that exists is
the one from assign_pages(). And that's the only one that
will get dropped when the domain gets cleaned up. The page,
in particular, hasn't been recorded in iorp just yet. So I
don't see where any other put (general or type) would come
from.

>> IOW
>> I'm not (yet) convinced you don't make the impact more
>> severe in the (supposedly) impossible case of these paths
>> getting hit, by converting a domain crash into a host one.
> 
> I have test cases.  I didn't go searching for the BUG()s by code inspection.

I'd be curious to see what exactly they do, and why exactly
a BUG() results.

>> It is in particular relevant that a PV guest may be able to
>> cheat and "guess" an MFN to obtain references for before a
>> certain hypercall (or other operation) actually completed.
> 
> And do what with it?  PV guest's can't force typerefs for
> pgtable/segdesc because we prohibited cross-pg_owner scenarios many XSAs
> ago.  A PV guest also can't force it to none or shared.

Hmm, yes, the owning domain always is a HVM one. So even by
cooperating the type can't become other than PGT_writable_page.

> That only leaves PGT_writable_page, which is the ref we're interested in
> taking.
> 
>>> v2:
>>>  * Reword the commit message.
>>>  * Switch BUG() to BUG_ON() to further reduce code volume.
>> Short of us explicitly agreeing that such is fine to use, I
>> think we ought to stick to the previously (long ago) agreed
>> rule that BUG_ON() controlling expressions should not have
>> side effects, for us not wanting to guarantee it will now
>> and forever _not_ behave like ASSERT() wrt to evaluating
>> the expression.
> 
> So what you want is v1 of this patch.

Looks like so (assuming my concerns above can get sorted); the
versions came in so rapid succession that I didn't get around
to look at the earlier versions.

Jan