Re: [PATCH] KVM: arm64: pkvm: Rollback refcount on hyp share/unshare error

[Vincent Donnefort <vdonnefort@google.com>]

On Mon, Mar 30, 2026 at 09:41:01AM +0000, Quentin Perret wrote:
> Hey Vincent,
> 
> On Tuesday 24 Mar 2026 at 17:27:57 (+0000), Vincent Donnefort wrote:
> > If one of the HVC __pkvm_host_share_hyp or __pkvm_host_unshare_hyp fails,
> > rollback the refcount to ensure the hyp_shared_pfns tracking reflects
> > the actual sharing status.
> 
> If any of these hypercalls fail I think we're still in trouble as
> kvm_{un}share_hyp() work on multi-page ranges and we could leak pages in
> a borked state if we fail halfway through. And failing any of these
> hypercalls is also sign of a bigger problem somewhere else so I wasn't
> too worried.

Yes, my bad, I haven't made that clear in the commit message: a failed HVC
right now is very much unlikely. I meant more to future proof and this isn't
fixing an existing corner case. 

> 
> But if we're going to fix this properly, I'd suggest also improving the
> error handling in kvm_share_hyp(). 'Fixing' kvm_unshare_hyp() is a bit
> harder because we must tell the caller to leak the data structure that
> was shared I presume, so maybe we just keep the WARN and cross our
> fingers :)

ack

> 
> Cheers,
> Quentin
> 
> > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > 
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 17d64a1e11e5..0fb41d2c8b44 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -493,11 +493,17 @@ static int share_pfn_hyp(u64 pfn)
> >  		goto unlock;
> >  	}
> >  
> > +	ret = kvm_call_hyp_nvhe(__pkvm_host_share_hyp, pfn);
> > +	if (ret) {
> > +		kfree(this);
> > +		goto unlock;
> > +	}
> > +
> >  	this->pfn = pfn;
> >  	this->count = 1;
> >  	rb_link_node(&this->node, parent, node);
> >  	rb_insert_color(&this->node, &hyp_shared_pfns);
> > -	ret = kvm_call_hyp_nvhe(__pkvm_host_share_hyp, pfn);
> > +
> >  unlock:
> >  	mutex_unlock(&hyp_shared_pfns_lock);
> >  
> > @@ -521,9 +527,15 @@ static int unshare_pfn_hyp(u64 pfn)
> >  	if (this->count)
> >  		goto unlock;
> >  
> > +	ret = kvm_call_hyp_nvhe(__pkvm_host_unshare_hyp, pfn);
> > +	if (ret) {
> > +		this->count++;
> > +		goto unlock;
> > +	}
> > +
> >  	rb_erase(&this->node, &hyp_shared_pfns);
> >  	kfree(this);
> > -	ret = kvm_call_hyp_nvhe(__pkvm_host_unshare_hyp, pfn);
> > +
> >  unlock:
> >  	mutex_unlock(&hyp_shared_pfns_lock);
> >  
> > 
> > base-commit: c369299895a591d96745d6492d4888259b004a9e
> > -- 
> > 2.53.0.1018.g2bb0e51243-goog
> >