Re: [PATCH v5 6/9] KVM: Add a helper function to check if a gpa is in writable memselot

From: Sean Christopherson <seanjc@google.com>
To: Atish Kumar Patra <atishp@rivosinc.com>
Cc: Anup Patel <anup@brainfault.org>, Will Deacon <will@kernel.org>,
	 Mark Rutland <mark.rutland@arm.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Mayuresh Chitale <mchitale@ventanamicro.com>,
	 linux-riscv@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	 linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	 kvm-riscv@lists.infradead.org
Subject: Re: [PATCH v5 6/9] KVM: Add a helper function to check if a gpa is in writable memselot
Date: Fri, 5 Sep 2025 01:23:17 -0700	[thread overview]
Message-ID: <aLqd9bKB6ucarR3e@google.com> (raw)
In-Reply-To: <CAHBxVyHFkNtFdX-vciPvYnTOH=GXvHVW7hjFrLA4MFr9wqWVvQ@mail.gmail.com>

On Wed, Sep 03, 2025, Atish Kumar Patra wrote:
> On Fri, Aug 29, 2025 at 1:47 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Aug 29, 2025, Atish Patra wrote:
> > > +static inline bool kvm_is_gpa_in_writable_memslot(struct kvm *kvm, gpa_t gpa)
> > > +{
> > > +     bool writable;
> > > +     unsigned long hva = gfn_to_hva_prot(kvm, gpa_to_gfn(gpa), &writable);
> > > +
> > > +     return !kvm_is_error_hva(hva) && writable;
> >
> > I don't hate this API, but I don't love it either.  Because knowing that the
> > _memslot_ is writable doesn't mean all that much.  E.g. in this usage:
> >
> >         hva = kvm_vcpu_gfn_to_hva_prot(vcpu, shmem >> PAGE_SHIFT, &writable);
> >         if (kvm_is_error_hva(hva) || !writable)
> >                 return SBI_ERR_INVALID_ADDRESS;
> >
> >         ret = kvm_vcpu_write_guest(vcpu, shmem, &zero_sta, sizeof(zero_sta));
> >         if (ret)
> >                 return SBI_ERR_FAILURE;
> >
> > the error code returned to the guest will be different if the memslot is read-only
> > versus if the VMA is read-only (or not even mapped!).  Unless every read-only
> > memslot is explicitly communicated as such to the guest, I don't see how the guest
> > can *know* that a memslot is read-only, so returning INVALID_ADDRESS in that case
> > but not when the underlying VMA isn't writable seems odd.
> >
> > It's also entirely possible the memslot could be replaced with a read-only memslot
> > after the check, or vice versa, i.e. become writable after being rejected.  Is it
> > *really* a problem to return FAILURE if the guest attempts to setup steal-time in
> > a read-only memslot?  I.e. why not do this and call it good?
> >
> 
> Reposting the response as gmail converted my previous response as
> html. Sorry for the spam.
> 
> From a functionality pov, that should be fine. However, we have
> explicit error conditions for read only memory defined in the SBI STA
> specification[1].
> Technically, we will violate the spec if we return FAILURE instead of
> INVALID_ADDRESS for read only memslot.

But KVM is already violating the spec, as kvm_vcpu_write_guest() redoes the
memslot lookup and so could encounter a read-only memslot (if it races with
a memslot update), and because the underlying memory could be read-only even if
the memslot is writable.

Why not simply return SBI_ERR_INVALID_ADDRESS on kvm_vcpu_write_guest() failure?
The only downside of that is KVM will also return SBI_ERR_INVALID_ADDRESS if the
userspace mapping is completely missing, but AFAICT that doesn't seem to be an
outright spec violation.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv