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

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

[Vincent Donnefort]

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.

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

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

[Quentin Perret]

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.

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 :)

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
> 

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

[Vincent Donnefort]

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
> >