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:   Fri, 2 Dec 2022 17:01:33 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     David Woodhouse <dwmw2@infradead.org>
Cc:     Michal Luczaj <mhal@rbox.co>, Like Xu <like.xu.linux@gmail.com>,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v2 05/16] KVM: x86: Remove unused argument in
 gpc_unmap_khva()
Message-ID: <Y4ovbTiLQ2Jy0em9@google.com>
References: <20221013211234.1318131-1-seanjc@google.com>
 <20221013211234.1318131-6-seanjc@google.com>
 <544d854f-e980-c7bf-39e9-425202760cc5@gmail.com>
 <6a984015-cacc-b8e9-d834-9d6827be5832@rbox.co>
 <ffc971cc6166528de7c33c30d190ece5eaee3dbf.camel@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ffc971cc6166528de7c33c30d190ece5eaee3dbf.camel@infradead.org>
Precedence: bulk

On Fri, Dec 02, 2022, David Woodhouse wrote:
> On Fri, 2022-12-02 at 11:57 +0100, Michal Luczaj wrote:
> > On 12/2/22 10:28, Like Xu wrote:
> > > On 14/10/2022 5:12 am, Sean Christopherson wrote:
> > > > Remove the unused @kvm argument from gpc_unmap_khva().
> > > 
> > > Nit: the caller kvm_gpc_unmap() can also get rid of the unused @kvm argument.
> > 
> > Right, the initial series cleaned up kvm_gpc_unmap() in a separate patch.
> > Current iteration removes kvm_gpc_unmap() later in the series:
> > https://lore.kernel.org/kvm/20221013211234.1318131-12-seanjc@google.com/
> 
> I have been keeping that series up to date in
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/gpc-fixes
> 
> Now that the dust has settled on the Xen runstate area, I may post it
> as v3 of the series.
> 
> Or I may attempt to resolve the gpc->len immutability thing first. I'm
> still not really convinced Sean has won me round on that;

Ya, I agree that storing "len" is undesirable, but so is storing "gpa" instead of
"gfn".

> I'm still quite attached to the TOCTOU benefit of checking the length right
> there at the moment you're going to use the pointer — especially given that
> it *doesn't* have bounds checks like get_user() does, as Sean points out.

I'm in favor of keeping the length checks if we modify the cache to store the
gfn, not the gpa, and require the gpa (or maybe just offset?) in the "get a kernel
pointer" API.

So, how about this for a set of APIs?  Obviously not tested whatsoever, but I
think they address the Xen use cases, and will fit the nested virt cases too
(which want to stuff a pfn into a VMCS/VMCB).

void *kvm_gpc_get_kmap(struct gfn_to_pfn_cache *gpc, gpa_t offset,
		       unsigned long len, bool atomic)
{
	<lock + refresh>

	return gpc->khva + offset;
}
EXPORT_SYMBOL_GPL(kvm_gpc_refresh);

kvm_pfn_t kvm_gpc_get_pfn(struct gfn_to_pfn_cache *gpc, bool atomic)
{
	<lock + refresh of full page>

	return gpc->pfn;
}
EXPORT_SYMBOL_GPL(kvm_gpc_refresh);

void kvm_gpc_put(struct gfn_to_pfn_cache *gpc)
{
	<unlock>
}
EXPORT_SYMBOL_GPL(kvm_gpc_refresh);

int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc)
{
	return __kvm_gpc_refresh(gpc, gfn_to_gpa(gpc->gfn), PAGE_SIZE);
}
EXPORT_SYMBOL_GPL(kvm_gpc_refresh);


And then __kvm_gpc_refresh() would do something like:

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 2d6aba677830..b2dd2eda4b56 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -236,22 +236,19 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
        return -EFAULT;
 }
 
-static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
+static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpt_t gpa,
                             unsigned long len)
 {
        struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
-       unsigned long page_offset = gpa & ~PAGE_MASK;
        bool unmap_old = false;
        unsigned long old_uhva;
        kvm_pfn_t old_pfn;
        void *old_khva;
+       gfn_t gfn;
        int ret;
 
-       /*
-        * If must fit within a single page. The 'len' argument is
-        * only to enforce that.
-        */
-       if (page_offset + len > PAGE_SIZE)
+       /* An individual cache doesn't support page splits. */
+       if ((gpa & ~PAGE_MASK) + len > PAGE_SIZE)
                return -EINVAL;
 
        /*
@@ -268,16 +265,16 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
                goto out_unlock;
        }
 
+       gfn = gpa_to_gfn(gpa);
+
        old_pfn = gpc->pfn;
-       old_khva = gpc->khva - offset_in_page(gpc->khva);
+       old_khva = gpc->khva;
        old_uhva = gpc->uhva;
 
        /* If the userspace HVA is invalid, refresh that first */
-       if (gpc->gpa != gpa || gpc->generation != slots->generation ||
+       if (gpc->gfn != gfn || gpc->generation != slots->generation ||
            kvm_is_error_hva(gpc->uhva)) {
-               gfn_t gfn = gpa_to_gfn(gpa);
-
-               gpc->gpa = gpa;
+               gpc->gfn = gfn;
                gpc->generation = slots->generation;
                gpc->memslot = __gfn_to_memslot(slots, gfn);
                gpc->uhva = gfn_to_hva_memslot(gpc->memslot, gfn);
@@ -295,12 +292,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
        if (!gpc->valid || old_uhva != gpc->uhva) {
                ret = hva_to_pfn_retry(gpc);
        } else {
-               /*
-                * If the HVA→PFN mapping was already valid, don't unmap it.
-                * But do update gpc->khva because the offset within the page
-                * may have changed.
-                */
-               gpc->khva = old_khva + page_offset;
+               /* If the HVA→PFN mapping was already valid, don't unmap it. */
                ret = 0;
                goto out_unlock;
        }