[PATCH v2 8/8] KVM: s390: Fix KVM_S390_VCPU_FAULT ioctl

Claudio Imbrenda posted 8 patches 2 weeks ago
There is a newer version of this series
[PATCH v2 8/8] KVM: s390: Fix KVM_S390_VCPU_FAULT ioctl
Posted by Claudio Imbrenda 2 weeks ago
A previous commit changed the behaviour of the KVM_S390_VCPU_FAULT
ioctl. The current (wrong) implementation will trigger a guest
addressing exception if the requested address lies outside of a
memslot, unless the VM is UCONTROL.

Restore the previous behaviour by open coding the fault-in logic.

Fixes: 3762e905ec2e ("KVM: s390: use __kvm_faultin_pfn()")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ebcb0ef8835e..62f04931b54d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5520,9 +5520,21 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	}
 #endif
 	case KVM_S390_VCPU_FAULT: {
-		idx = srcu_read_lock(&vcpu->kvm->srcu);
-		r = vcpu_dat_fault_handler(vcpu, arg, 0);
-		srcu_read_unlock(&vcpu->kvm->srcu, idx);
+		gpa_t gaddr = arg;
+
+		scoped_guard(srcu, &vcpu->kvm->srcu) {
+			r = vcpu_ucontrol_translate(vcpu, &gaddr);
+			if (r)
+				break;
+
+			r = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(gaddr), false);
+			if (r == PGM_ADDRESSING)
+				r = -EFAULT;
+			if (r <= 0)
+				break;
+			r = -EIO;
+			KVM_BUG_ON(r, vcpu->kvm);
+		}
 		break;
 	}
 	case KVM_ENABLE_CAP:
-- 
2.53.0
Re: [PATCH v2 8/8] KVM: s390: Fix KVM_S390_VCPU_FAULT ioctl
Posted by Christian Borntraeger 1 week, 4 days ago
Am 20.03.26 um 17:15 schrieb Claudio Imbrenda:
> A previous commit changed the behaviour of the KVM_S390_VCPU_FAULT
> ioctl. The current (wrong) implementation will trigger a guest
> addressing exception if the requested address lies outside of a
> memslot, unless the VM is UCONTROL.
> 
> Restore the previous behaviour by open coding the fault-in logic.
> 
> Fixes: 3762e905ec2e ("KVM: s390: use __kvm_faultin_pfn()")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Is this something that we could improve in the
tools/testing/selftests/kvm/s390/ucontrol_test.c
testcase? We do test KVM_S390_VCPU_FAULT in there.

Apart from that

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Re: [PATCH v2 8/8] KVM: s390: Fix KVM_S390_VCPU_FAULT ioctl
Posted by Steffen Eiden 1 week, 4 days ago
On Fri, Mar 20, 2026 at 05:15:42PM +0100, Claudio Imbrenda wrote:
> A previous commit changed the behaviour of the KVM_S390_VCPU_FAULT
> ioctl. The current (wrong) implementation will trigger a guest
> addressing exception if the requested address lies outside of a
> memslot, unless the VM is UCONTROL.
> 
> Restore the previous behaviour by open coding the fault-in logic.
> 
> Fixes: 3762e905ec2e ("KVM: s390: use __kvm_faultin_pfn()")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

But I have a comment on a changed logic. And a nit

> ---
>  arch/s390/kvm/kvm-s390.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ebcb0ef8835e..62f04931b54d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5520,9 +5520,21 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	}
>  #endif
>  	case KVM_S390_VCPU_FAULT: {
> -		idx = srcu_read_lock(&vcpu->kvm->srcu);
> -		r = vcpu_dat_fault_handler(vcpu, arg, 0);
	in here every vcpu_ucontrol_translate error (incl ENONEMs from
	kvm_s390_mmu_cache_topup) is converted into EREMOTE ...
> -		srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +		gpa_t gaddr = arg;
> +
> +		scoped_guard(srcu, &vcpu->kvm->srcu) {
> +			r = vcpu_ucontrol_translate(vcpu, &gaddr);
> +			if (r)
> +				break;
	... which is not longer the case here. As you explicitly convert
	ENOMENS in gmap_ucas_translate before the topup call tnot converting
	might be an overlook (in the topup function?).
> +
> +			r = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(gaddr), false);
> +			if (r == PGM_ADDRESSING)
> +				r = -EFAULT;
> +			if (r <= 0)
> +				break;
	nit: in vcpu_dat_fault_handler the ifs are in the inverse order.
	They are independent, so this does not make any difference, but this
	itches me a little. :) 

> +			r = -EIO;
> +			KVM_BUG_ON(r, vcpu->kvm);
> +		}
>  		break;
>  	}
>  	case KVM_ENABLE_CAP:
> -- 
> 2.53.0
> 
	Steffen
Re: [PATCH v2 8/8] KVM: s390: Fix KVM_S390_VCPU_FAULT ioctl
Posted by Janosch Frank 1 week, 3 days ago
On 3/23/26 12:10, Steffen Eiden wrote:
> On Fri, Mar 20, 2026 at 05:15:42PM +0100, Claudio Imbrenda wrote:
>> A previous commit changed the behaviour of the KVM_S390_VCPU_FAULT
>> ioctl. The current (wrong) implementation will trigger a guest
>> addressing exception if the requested address lies outside of a
>> memslot, unless the VM is UCONTROL.
>>
>> Restore the previous behaviour by open coding the fault-in logic.
>>
>> Fixes: 3762e905ec2e ("KVM: s390: use __kvm_faultin_pfn()")
>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> 
> But I have a comment on a changed logic. And a nit
> 
>> ---
>>   arch/s390/kvm/kvm-s390.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index ebcb0ef8835e..62f04931b54d 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -5520,9 +5520,21 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>   	}
>>   #endif
>>   	case KVM_S390_VCPU_FAULT: {
>> -		idx = srcu_read_lock(&vcpu->kvm->srcu);
>> -		r = vcpu_dat_fault_handler(vcpu, arg, 0);
> 	in here every vcpu_ucontrol_translate error (incl ENONEMs from
> 	kvm_s390_mmu_cache_topup) is converted into EREMOTE ...
>> -		srcu_read_unlock(&vcpu->kvm->srcu, idx);
>> +		gpa_t gaddr = arg;
>> +
>> +		scoped_guard(srcu, &vcpu->kvm->srcu) {
>> +			r = vcpu_ucontrol_translate(vcpu, &gaddr);
>> +			if (r)
>> +				break;
> 	... which is not longer the case here. As you explicitly convert
> 	ENOMENS in gmap_ucas_translate before the topup call tnot converting
> 	might be an overlook (in the topup function?).

Yeah, I second that.