Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 17 Jan 2023 15:55:53 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Zhi Wang <zhi.wang.linux@gmail.com>
Cc:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
        Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
        Sagi Shahar <sagis@google.com>,
        David Matlack <dmatlack@google.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Kai Huang <kai.huang@intel.com>
Subject: Re: [PATCH v11 018/113] KVM: TDX: create/destroy VM structure
Message-ID: <Y8bFCb+rs25dKcMY@google.com>
References: <cover.1673539699.git.isaku.yamahata@intel.com>
 <68fa413e61d7471657174bc7c83bde5c842e251f.1673539699.git.isaku.yamahata@intel.com>
 <20230113151258.00006a6d@gmail.com>
 <Y8F1uPsW56fVdhmC@google.com>
 <20230114111621.00001840@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230114111621.00001840@gmail.com>
Precedence: bulk

On Sat, Jan 14, 2023, Zhi Wang wrote:
> On Fri, 13 Jan 2023 15:16:08 +0000 > Sean Christopherson <seanjc@google.com> wrote:
> 
> > On Fri, Jan 13, 2023, Zhi Wang wrote:
> > > Better add a FIXME: here as this has to be fixed later.
> > 
> > No, leaking the page is all KVM can reasonably do here.  An improved
> > comment would be helpful, but no code change is required.
> > tdx_reclaim_page() returns an error if and only if there's an
> > unexpected, fatal error, e.g. a SEAMCALL with bad params, incorrect
> > concurrency in KVM, a TDX Module bug, etc.  Retrying at a later point is
> > highly unlikely to be successful.
> 
> Hi:
> 
> The word "leaking" sounds like a situation left unhandled temporarily.
> 
> I checked the source code of the TDX module[1] for the possible reason to
> fail when reviewing this patch:
> 
> tdx-module-v1.0.01.01.zip\src\vmm_dispatcher\api_calls\tdh_phymem_page_reclaim.c
> tdx-module-v1.0.01.01.zip\src\vmm_dispatcher\api_calls\tdh_phymem_page_wbinvd.c
> 
> a. Invalid parameters. For example, page is not aligned, PA HKID is not zero...
> 
> For invalid parameters, a WARN_ON_ONCE() + return value is good enough as
> that is how kernel handles similar situations. The caller takes the
> responsibility.
>  
> b. Locks has been taken in TDX module. TDR page has been locked due to another
> SEAMCALL, another SEAMCALL is doing PAMT walk and holding PAMT lock... 
> 
> This needs to be improved later either by retry or taking tdx_lock to avoid
> TDX module fails on this.

No, tdx_reclaim_page() already retries TDH.PHYMEM.PAGE.RECLAIM if the target page
is contended (though I'd question the validity of even that), and TDH.PHYMEM.PAGE.WBINVD
is performed only when reclaiming the TDR.  If there's contention when reclaiming
the TDR, then KVM effectively has a use-after-free bug, i.e. leaking the page is
the least of our worries.


On Thu, Jan 12, 2023 at 8:34 AM <isaku.yamahata@intel.com> wrote:
> +static int tdx_reclaim_page(hpa_t pa, bool do_wb, u16 hkid)
> +{
> + ? ? ? struct tdx_module_output out;
> + ? ? ? u64 err;
> +
> + ? ? ? do {
> + ? ? ? ? ? ? ? err = tdh_phymem_page_reclaim(pa, &out);
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* TDH.PHYMEM.PAGE.RECLAIM is allowed only when TD is shutdown.
> + ? ? ? ? ? ? ? ?* state. ?i.e. destructing TD.
> + ? ? ? ? ? ? ? ?* TDH.PHYMEM.PAGE.RECLAIM ?requires TDR and target page.
> + ? ? ? ? ? ? ? ?* Because we're destructing TD, it's rare to contend with TDR.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? } while (err == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_RCX));
> + ? ? ? if (WARN_ON_ONCE(err)) {
> + ? ? ? ? ? ? ? pr_tdx_error(TDH_PHYMEM_PAGE_RECLAIM, err, &out);
> + ? ? ? ? ? ? ? return -EIO;
> + ? ? ? }
> +
> + ? ? ? if (do_wb) {
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* Only TDR page gets into this path. ?No contention is expected
> + ? ? ? ? ? ? ? ?* because of the last page of TD.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? err = tdh_phymem_page_wbinvd(set_hkid_to_hpa(pa, hkid));
> + ? ? ? ? ? ? ? if (WARN_ON_ONCE(err)) {
> + ? ? ? ? ? ? ? ? ? ? ? pr_tdx_error(TDH_PHYMEM_PAGE_WBINVD, err, NULL);
> + ? ? ? ? ? ? ? ? ? ? ? return -EIO;
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> +
> + ? ? ? tdx_clear_page(pa);
> + ? ? ? return 0;
> +}