Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7fadab86-2b7c-b934-fcfa-61046c0778b6@linux.ibm.com>
Date:   Fri, 28 Jul 2023 09:28:58 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] KVM: s390: fix sthyi error handling
To:     Heiko Carstens <hca@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        Mete Durlu <meted@linux.ibm.com>
Cc:     kvm@vger.kernel.org, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230727182939.2050744-1-hca@linux.ibm.com>
Content-Language: en-US
From:   Christian Borntraeger <borntraeger@linux.ibm.com>
In-Reply-To: <20230727182939.2050744-1-hca@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@linux.ibm.com>


> 
> Fixes: 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info")
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>   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)) {