Re: [PATCH] KVM: s390: fix sthyi error handling

[Christian Borntraeger]

Am 27.07.23 um 20:29 schrieb Heiko Carstens:
> Commit 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info")
> added cache handling for store hypervisor info. This also changed the
> possible return code for sthyi_fill().
>
> Instead of only returning a condition code like the sthyi instruction would
> do, it can now also return a negative error value (-ENOMEM). handle_styhi()
> was not changed accordingly. In case of an error, the negative error value
> would incorrectly injected into the guest PSW.
>
> Add proper error handling to prevent this, and update the comment which
> describes the possible return values of sthyi_fill().

To me it looks like this can only happen if page allocation fails? This should
not happen in normal cases (and return -ENOMEM would likely kill the guest as
QEMU would stop).
But if it happens we better stop.


Reviewed-by: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>


> Fixes: 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info")
> Signed-off-by: Heiko Carstens <hca@xxxxxxxxxxxxx>
> ---
>   arch/s390/kernel/sthyi.c  | 6 +++---
>   arch/s390/kvm/intercept.c | 9 ++++++---
>   2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c
> index 4d141e2c132e..2ea7f208f0e7 100644
> --- a/arch/s390/kernel/sthyi.c
> +++ b/arch/s390/kernel/sthyi.c
> @@ -459,9 +459,9 @@ static int sthyi_update_cache(u64 *rc)
>    *
>    * Fills the destination with system information returned by the STHYI
>    * instruction. The data is generated by emulation or execution of STHYI,
> - * if available. The return value is the condition code that would be
> - * returned, the rc parameter is the return code which is passed in
> - * register R2 + 1.
> + * if available. The return value is either a negative error value or
> + * the condition code that would be returned, the rc parameter is the
> + * return code which is passed in register R2 + 1.
>    */
>   int sthyi_fill(void *dst, u64 *rc)
>   {
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 954d39adf85c..341abafb96e4 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -389,8 +389,8 @@ static int handle_partial_execution(struct kvm_vcpu *vcpu)
>    */
>   int handle_sthyi(struct kvm_vcpu *vcpu)
>   {
> -	int reg1, reg2, r = 0;
> -	u64 code, addr, cc = 0, rc = 0;
> +	int reg1, reg2, cc = 0, r = 0;
> +	u64 code, addr, rc = 0;
>   	struct sthyi_sctns *sctns = NULL;
>   
>   	if (!test_kvm_facility(vcpu->kvm, 74))
> @@ -421,7 +421,10 @@ int handle_sthyi(struct kvm_vcpu *vcpu)
>   		return -ENOMEM;
>   
>   	cc = sthyi_fill(sctns, &rc);
> -
> +	if (cc < 0) {
> +		free_page((unsigned long)sctns);
> +		return cc;
> +	}
>   out:
>   	if (!cc) {
>   		if (kvm_s390_pv_cpu_is_protected(vcpu)) {