[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Fri, 29 Jan 2021 17:17:54 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=P/WziYJlfW5rOVVxK2h87Ia/8Zm3rIGwzLaZyARQu7s=; b=D9uEPJnJjxaYz+JDqqf1w6+imXVqJII06VMupxcSQQ2Zs1QeQkPCaa+nlw5k5J8X4pMUH3+kA3swx1hHVAw3bTxyrw+8zzC9E8XjuFxFFTLJvyMOqCmjRxMVCh0wfMlMLdWrxsgbIvPq38gEHxjx7PFZ8ePT4zDqI2IYztfFGs7O7pL79mWehLk//cTRIS59oBWj6hztOBE0+UrM8h1WliIVkT3XMS55R+8nH4rHAGfLkQgzutdtuGgn9+DfdXZBoR5NrbYQWId4W13EFfXN6u1DbrX/nbv1Zro5vriesXzbxXz8/c5pdkPD8xA8pnZIr5kEEU1oos+psFxEqVpuzQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=oSih3rvMCcolKhNoAfeQB1ja63Gbe9OqYILd1KSeXbwV1KT3MOnPGQdIlEFV5xN3Gjcsc77Mx8BsDllsTY7QBTWhvWsGNAt6oedDNxTNQgYvBmjVRmoWxqmeN7b1XAwDOhTlDbtQPTGUdBOHVrxqipcwx+vWknfErXhIDOhnsgJsjTMhwB1rB5UlhvYMGEuQtRXseoIhCvGct6QFdGC6rRIh4KR0cyCFJVwvR1PK3wnwToQpOtxG+P9K8FhqeDI0w31T8JJfhU5LEKq15AYtOre+WQZ9N02YEKGpeIrF77Usn4+sndvaL5DIf1XMh4wqvc/wkEJoLQeMmnhC9ntdpQ==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 29 Jan 2021 17:18:23 +0000
  • Ironport-sdr: qRnyIQ4HBN0O+f3NXWjipA4ukToS4tpefb5NGx5gnMH3KBS3Yw+C8zSXrTqTxWU/gl2anTbtQJ 9rHQJ0z2HgSCjvFPMCzOtH32CyL/ECrfEb59yyr2hkSgGsVO4/6BO0OF83yf5tDMHAdo3yjSO5 T0jqepL2ktAokrHb8FqubdoUgjFL58LMSUh2BqhCNdZj7bNCGEyGjx4NDM7W++wIr0JZsTIoH8 +5gQtI3ANwiKylIyO2YP2kOLF4g67aBxyVBG8GMX4OdhAZ53dr7TSS4+cwudWN6g+DcxXB5yGG 5Yk=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 29/01/2021 16:31, Jan Beulich wrote:
> On 29.01.2021 17:17, Andrew Cooper wrote:
>> On 29/01/2021 11:29, Jan Beulich wrote:
>>> 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.
>> The refcounting logic was taken verbatim from ioreq, with the only
>> difference being an order greater than 0.  The logic is also identical
>> to the vlapic logic.
>> And the reason *why* it bugs is obvious - the cleanup logic
>> unconditionally put()'s refs it never took to begin with, and hits
>> underflow bugchecks.
> In current staging, neither vmx_alloc_vlapic_mapping() nor
> hvm_alloc_ioreq_mfn() put any refs they couldn't get. Hence
> my failed attempt to repro your claimed misbehavior.

I think I've figured out what is going on.

They *look* as if they do, but the logic is deceptive.

We skip both puts in free_*() if the typeref failed, and rely on the
fact that the frame(s) are *also* on the domheap list for
relinquish_resources() to put the acquire ref.

Yet another bizzare recounting rule/behaviour which isn't written down.

My bug really was setting v->vmtrace.buf too early.  Furthermore, this
pattern is not safe for use in the domain_create() path, because it
currently depends on the domain being put on the domain list and
relinquish_resources() being called to avoid leaking memory.

I still argue that a typeref failure in these circumstances is the same
kind of impossibility that we use BUG for elsewhere, and therefore that
is what we should be doing.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.