From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 91AF3C433E0 for ; Mon, 1 Feb 2021 14:36:38 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 557F764DDE for ; Mon, 1 Feb 2021 14:36:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 557F764DDE Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.79916.145750 (Exim 4.92) (envelope-from ) id 1l6aJY-00085F-07; Mon, 01 Feb 2021 14:36:28 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 79916.145750; Mon, 01 Feb 2021 14:36:27 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1l6aJX-000858-TD; Mon, 01 Feb 2021 14:36:27 +0000 Received: by outflank-mailman (input) for mailman id 79916; Mon, 01 Feb 2021 14:36:26 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1l6aJW-000851-6U for xen-devel@lists.xenproject.org; Mon, 01 Feb 2021 14:36:26 +0000 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 5eaea734-a8c9-49da-8cf6-ba2e7234efb8; Mon, 01 Feb 2021 14:36:25 +0000 (UTC) Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 584B7AB92; Mon, 1 Feb 2021 14:36:24 +0000 (UTC) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 5eaea734-a8c9-49da-8cf6-ba2e7234efb8 X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1612190184; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6uykghAdfz9re78oxMr0Ju2czSjC0DFr3lBX48I5rJY=; b=UIRXS0qGyOKD43/9h6/BoC469Q11Ymo2/Ak3rTprFlzqc/Xg3Lk+HmpH3SSyV3MOCJ9sUX QKZYoTu/LowwuyVQFrdLgk/sktMbeMex7CsRS4oaThOSA38aI0YXkIKBP6fPgXInvJEuuZ GmO2i6+Gn6b4tQzEx+7lazHOg8hUj4k= Subject: Re: [PATCH v8 08/16] xen/domain: Add vmtrace_size domain creation parameter To: Andrew Cooper Cc: =?UTF-8?Q?Micha=c5=82_Leszczy=c5=84ski?= , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= , Wei Liu , Anthony PERARD , Tamas K Lengyel , Xen-devel References: <20210130025852.12430-1-andrew.cooper3@citrix.com> <20210130025852.12430-9-andrew.cooper3@citrix.com> <3cf886f6-db7f-ccc1-5ef0-6fd8ccb38caf@suse.com> From: Jan Beulich Message-ID: <296e5ee3-0ae1-fe0b-9ec3-940b78284cdc@suse.com> Date: Mon, 1 Feb 2021 15:36:23 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 01.02.2021 15:22, Andrew Cooper wrote: > On 01/02/2021 13:18, Jan Beulich wrote: >> On 30.01.2021 03:58, Andrew Cooper wrote: >>> +static int vmtrace_alloc_buffer(struct vcpu *v) >>> +{ >>> + struct domain *d = v->domain; >>> + struct page_info *pg; >>> + unsigned int i; >>> + >>> + if ( !d->vmtrace_size ) >>> + return 0; >>> + >>> + pg = alloc_domheap_pages(d, get_order_from_bytes(d->vmtrace_size), >>> + MEMF_no_refcount); >>> + if ( !pg ) >>> + return -ENOMEM; >>> + >>> + /* >>> + * Getting the reference counting correct here is hard. >>> + * >>> + * All pages are now on the domlist. They, or subranges within, will be >> "domlist" is too imprecise, as there's no list with this name. It's >> extra_page_list in this case (see also below). >> >>> + * freed when their reference count drops to zero, which may any time >>> + * between now and the domain teardown path. >>> + */ >>> + >>> + for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ ) >>> + if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) ) >>> + goto refcnt_err; >>> + >>> + /* >>> + * We must only let vmtrace_free_buffer() take any action in the success >>> + * case when we've taken all the refs it intends to drop. >>> + */ >>> + v->vmtrace.pg = pg; >>> + >>> + return 0; >>> + >>> + refcnt_err: >>> + /* >>> + * In the failure case, we must drop all the acquired typerefs thus far, >>> + * skip vmtrace_free_buffer(), and leave domain_relinquish_resources() to >>> + * drop the alloc refs on any remaining pages - some pages could already >>> + * have been freed behind our backs. >>> + */ >>> + while ( i-- ) >>> + put_page_and_type(&pg[i]); >>> + >>> + return -ENODATA; >>> +} >> As said in reply on the other thread, PGC_extra pages don't get >> freed automatically. I too initially thought they would, but >> (re-)learned otherwise when trying to repro your claims on that >> other thread. For all pages you've managed to get the writable >> ref, freeing is easily done by prefixing the loop body above by >> put_page_alloc_ref(). For all other pages best you can do (I >> think; see the debugging patches I had sent on that other >> thread) is to try get_page() - if it succeeds, calling >> put_page_alloc_ref() is allowed. Otherwise we can only leak the >> respective page (unless going to further extents with trying to >> recover from the "impossible"), or assume the failure here was >> because it did get freed already. > > Right - I'm going to insist on breaking apart orthogonal issues. > > This refcounting issue isn't introduced by this series - this series > uses an established pattern, in which we've found a corner case. > > The corner case is theoretical, not practical - it is not possible for a > malicious PV domain to take 2^43 refs on any of the pages in this > allocation.  Doing so would require an hours-long SMI, or equivalent, > and even then all malicious activity would be paused after 1s for the > time calibration rendezvous which would livelock the system until the > watchdog kicked in. Actually an overflow is only one of the possible reasons here. Another, which may be more "practical", is that another entity has already managed to free the page (by dropping its alloc-ref, and of course implying it did guess at the MFN). Jan