Re: [PATCH v2 3/3] KVM: Take gpa_t in kvm_vcpu_map[_readonly]()

Sean Christopherson
2026-04-22T20:34:31Z

On Wed, Apr 22, 2026, Yosry Ahmed wrote:
> > > Perhaps we just need to rename the functions (e.g.
> > > kvm_vcpu_map_page()), or more intrusively pass in a size and do bounds
> > > checking.
> >
> > Definitely the latter. Or both I guess, but probably just the latter.
>
> I think both. I think renaming to kvm_vcpu_map_page() (and similar for
> others) would further clarify things, especially with the introduction
> of kvm_vcpu_map_ptr() below.

I don't like "page" it's too easy to incorrectly assume "page" means "struct page".
There are KVM APIs that do use "page" generically, e.g. kvm_read_guest_page(),
but for this particular case I'd like to stay away from "page; there's a _lot_
of ugly history around mapping "struct page" vs. "other" memory in KVM.

> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 9093251beb39..e8d2e98b0068 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -3114,9 +3114,10 @@ struct page *__gfn_to_page(struct kvm *kvm, gfn_t gfn, bool write)
> > }
> > EXPORT_SYMBOL_FOR_KVM_INTERNAL(__gfn_to_page);
> >
> > -int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map,
> > - bool writable)
> > +int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gpa_t gpa, gpa_t len,
> > + struct kvm_host_map *map, bool writable)
> > {
> > + gfn_t gfn = gpa_to_gfn(gpa);
> > struct kvm_follow_pfn kfp = {
> > .slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn),
> > .gfn = gfn,
> > @@ -3124,6 +3125,10 @@ int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map,
> > .refcounted_page = &map->pinned_page,
> > .pin = true,
> > };
> > + kvm_pfn_t pfn;
> > +
> > + if (WARN_ON_ONCE(offset_in_page(gpa) + len > PAGE_SIZE))
> > + return -EINVAL;
>
> Maybe do the bounds checking after initializing 'map', then
> kvm_vcpu_map_ptr() wouldn't need to explicitly set the pointer to NULL
> on failure?

Hmm, no. I don't want to encourage the caller to rely on the state of @map if
the call fails.

> There is already possibility of failure after initialization anyway.

Sure, but the caller shouldn't rely on that.