Re: [PATCH v3 10/10] KVM: s390: Fix cleanup in kvm_s390_pv_create_cpu()

[Claudio Imbrenda]

On Tue, 28 Jul 2026 16:12:15 +0200
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:

> On 7/27/26 17:02, Claudio Imbrenda wrote:
> > If creating a protected vCPU in kvm_s390_pv_create_cpu() fails,
> > kvm_s390_pv_destroy_cpu() was called, which checks whether the vCPU has
> > a PV handle and exits doing nothing otherwise. At that point, due to
> > not having created the protected vCPU, the PV handle will not be set,
> > and kvm_s390_pv_destroy_cpu() will do nothing, thus leaking the
> > allocated memory.
> > 
> > Fix by factoring out the code to free and reset a PV vCPU; call it from
> > kvm_s390_pv_destroy_cpu() and kvm_s390_pv_create_cpu().
> > 
> > Fixes: d4074324b07a ("KVM: s390: pv: avoid double free of sida page")
> > Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > ---
> >   arch/s390/kvm/pv.c | 41 +++++++++++++++++++++--------------------
> >   1 file changed, 21 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> > index dc204b521052..b02e0159d3cd 100644
> > --- a/arch/s390/kvm/pv.c
> > +++ b/arch/s390/kvm/pv.c
> > @@ -244,6 +244,24 @@ static void kvm_s390_clear_pv_state(struct kvm *kvm)
> >   	kvm->arch.pv.stor_var = NULL;
> >   }
> >   
> > +static void kvm_s390_pv_dispose_cpu(struct kvm_vcpu *vcpu, bool free_stor_base)
> > +{
> > +	if (free_stor_base)
> > +		free_pages(vcpu->arch.pv.stor_base, get_order(uv_info.guest_cpu_stor_len));
> > +	free_page((unsigned long)sida_addr(vcpu->arch.sie_block));
> > +	vcpu->arch.sie_block->pv_handle_cpu = 0;
> > +	vcpu->arch.sie_block->pv_handle_config = 0;
> > +	memset(&vcpu->arch.pv, 0, sizeof(vcpu->arch.pv));
> > +	vcpu->arch.sie_block->sdf = 0;
> > +	/*
> > +	 * The sidad field (for sdf == 2) is now the gbea field (for sdf == 0).
> > +	 * Use the reset value of gbea to avoid leaking the kernel pointer of
> > +	 * the just freed sida.
> > +	 */
> > +	vcpu->arch.sie_block->gbea = 1;
> > +	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> > +}
> > +
> >   int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
> >   {
> >   	int cc;
> > @@ -258,24 +276,9 @@ int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
> >   	WARN_ONCE(cc, "protvirt destroy cpu failed rc %x rrc %x", *rc, *rrc);
> >   
> >   	/* Intended memory leak for something that should never happen. */  
> 
> That comment doesn't make a lot of sense anymore after your changes.

how so?
potentially calling the helper function with free_stor_base == false