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

[Sean Christopherson <seanjc@google.com>]

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.