All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: s390: Fix lockdep issue in vm memop
@ 2022-03-22 15:32 Janis Schoetterl-Glausch
  2022-03-22 16:15 ` Claudio Imbrenda
  2022-03-23  7:58 ` Janosch Frank
  0 siblings, 2 replies; 8+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-03-22 15:32 UTC (permalink / raw)
  To: Christian Borntraeger, Janosch Frank, Claudio Imbrenda,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
	Janis Schoetterl-Glausch
  Cc: David Hildenbrand, Sven Schnelle, kvm, linux-s390, linux-kernel

Issuing a memop on a protected vm does not make sense,
neither is the memory readable/writable, nor does it make sense to check
storage keys. This is why the ioctl will return -EINVAL when it detects
the vm to be protected. However, in order to ensure that the vm cannot
become protected during the memop, the kvm->lock would need to be taken
for the duration of the ioctl. This is also required because
kvm_s390_pv_is_protected asserts that the lock must be held.
Instead, don't try to prevent this. If user space enables secure
execution concurrently with a memop it must accecpt the possibility of
the memop failing.
Still check if the vm is currently protected, but without locking and
consider it a heuristic.

Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ca96f84db2cc..53adbe86a68f 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 		return -EINVAL;
 	if (mop->size > MEM_OP_MAX_SIZE)
 		return -E2BIG;
-	if (kvm_s390_pv_is_protected(kvm))
+	/*
+	 * This is technically a heuristic only, if the kvm->lock is not
+	 * taken, it is not guaranteed that the vm is/remains non-protected.
+	 * This is ok from a kernel perspective, wrongdoing is detected
+	 * on the access, -EFAULT is returned and the vm may crash the
+	 * next time it accesses the memory in question.
+	 * There is no sane usecase to do switching and a memop on two
+	 * different CPUs at the same time.
+	 */
+	if (kvm_s390_pv_get_handle(kvm))
 		return -EINVAL;
 	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
 		if (access_key_invalid(mop->key))

base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] KVM: s390: Fix lockdep issue in vm memop
  2022-03-22 15:32 [PATCH] KVM: s390: Fix lockdep issue in vm memop Janis Schoetterl-Glausch
@ 2022-03-22 16:15 ` Claudio Imbrenda
  2022-03-23  7:58 ` Janosch Frank
  1 sibling, 0 replies; 8+ messages in thread
From: Claudio Imbrenda @ 2022-03-22 16:15 UTC (permalink / raw)
  To: Janis Schoetterl-Glausch
  Cc: Christian Borntraeger, Janosch Frank, Heiko Carstens,
	Vasily Gorbik, Alexander Gordeev, David Hildenbrand,
	Sven Schnelle, kvm, linux-s390, linux-kernel

On Tue, 22 Mar 2022 16:32:04 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Issuing a memop on a protected vm does not make sense,
> neither is the memory readable/writable, nor does it make sense to check
> storage keys. This is why the ioctl will return -EINVAL when it detects
> the vm to be protected. However, in order to ensure that the vm cannot
> become protected during the memop, the kvm->lock would need to be taken
> for the duration of the ioctl. This is also required because
> kvm_s390_pv_is_protected asserts that the lock must be held.
> Instead, don't try to prevent this. If user space enables secure
> execution concurrently with a memop it must accecpt the possibility of
> the memop failing.
> Still check if the vm is currently protected, but without locking and
> consider it a heuristic.
> 
> Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ca96f84db2cc..53adbe86a68f 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>  		return -EINVAL;
>  	if (mop->size > MEM_OP_MAX_SIZE)
>  		return -E2BIG;
> -	if (kvm_s390_pv_is_protected(kvm))
> +	/*
> +	 * This is technically a heuristic only, if the kvm->lock is not
> +	 * taken, it is not guaranteed that the vm is/remains non-protected.
> +	 * This is ok from a kernel perspective, wrongdoing is detected
> +	 * on the access, -EFAULT is returned and the vm may crash the
> +	 * next time it accesses the memory in question.
> +	 * There is no sane usecase to do switching and a memop on two
> +	 * different CPUs at the same time.
> +	 */
> +	if (kvm_s390_pv_get_handle(kvm))
>  		return -EINVAL;
>  	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>  		if (access_key_invalid(mop->key))
> 
> base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] KVM: s390: Fix lockdep issue in vm memop
  2022-03-22 15:32 [PATCH] KVM: s390: Fix lockdep issue in vm memop Janis Schoetterl-Glausch
  2022-03-22 16:15 ` Claudio Imbrenda
@ 2022-03-23  7:58 ` Janosch Frank
  2022-03-23  8:52   ` Janis Schoetterl-Glausch
  1 sibling, 1 reply; 8+ messages in thread
From: Janosch Frank @ 2022-03-23  7:58 UTC (permalink / raw)
  To: Janis Schoetterl-Glausch, Christian Borntraeger,
	Claudio Imbrenda, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev
  Cc: David Hildenbrand, Sven Schnelle, kvm, linux-s390, linux-kernel

On 3/22/22 16:32, Janis Schoetterl-Glausch wrote:
> Issuing a memop on a protected vm does not make sense,

Issuing a vm memop on a protected vm...

The cpu memop still makes sense, no?

> neither is the memory readable/writable, nor does it make sense to check
> storage keys. This is why the ioctl will return -EINVAL when it detects
> the vm to be protected. However, in order to ensure that the vm cannot
> become protected during the memop, the kvm->lock would need to be taken
> for the duration of the ioctl. This is also required because
> kvm_s390_pv_is_protected asserts that the lock must be held.
> Instead, don't try to prevent this. If user space enables secure
> execution concurrently with a memop it must accecpt the possibility of
> the memop failing.
> Still check if the vm is currently protected, but without locking and
> consider it a heuristic.
> 
> Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Makes sense to me.

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>   arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ca96f84db2cc..53adbe86a68f 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>   		return -EINVAL;
>   	if (mop->size > MEM_OP_MAX_SIZE)
>   		return -E2BIG;
> -	if (kvm_s390_pv_is_protected(kvm))
> +	/*
> +	 * This is technically a heuristic only, if the kvm->lock is not
> +	 * taken, it is not guaranteed that the vm is/remains non-protected.
> +	 * This is ok from a kernel perspective, wrongdoing is detected
> +	 * on the access, -EFAULT is returned and the vm may crash the
> +	 * next time it accesses the memory in question.
> +	 * There is no sane usecase to do switching and a memop on two
> +	 * different CPUs at the same time.
> +	 */
> +	if (kvm_s390_pv_get_handle(kvm))
>   		return -EINVAL;
>   	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>   		if (access_key_invalid(mop->key))
> 
> base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] KVM: s390: Fix lockdep issue in vm memop
  2022-03-23  7:58 ` Janosch Frank
@ 2022-03-23  8:52   ` Janis Schoetterl-Glausch
  2022-03-23  8:57     ` Janosch Frank
  0 siblings, 1 reply; 8+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-03-23  8:52 UTC (permalink / raw)
  To: Janosch Frank, Christian Borntraeger, Claudio Imbrenda,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev
  Cc: David Hildenbrand, Sven Schnelle, kvm, linux-s390, linux-kernel

On 3/23/22 08:58, Janosch Frank wrote:
> On 3/22/22 16:32, Janis Schoetterl-Glausch wrote:
>> Issuing a memop on a protected vm does not make sense,
> 
> Issuing a vm memop on a protected vm...
> 
> The cpu memop still makes sense, no?

The vcpu memop does hold the vcpu->lock, so no lockdep issue.
If you issue a vcpu memop while enabling protected virtualization,
the memop might find that the vcpu is not protected, while other vcpus
might already be, but I don't think there's a way to create secure memory
concurrent with the memop.
> 
>> neither is the memory readable/writable, nor does it make sense to check
>> storage keys. This is why the ioctl will return -EINVAL when it detects
>> the vm to be protected. However, in order to ensure that the vm cannot
>> become protected during the memop, the kvm->lock would need to be taken
>> for the duration of the ioctl. This is also required because
>> kvm_s390_pv_is_protected asserts that the lock must be held.
>> Instead, don't try to prevent this. If user space enables secure
>> execution concurrently with a memop it must accecpt the possibility of
>> the memop failing.
>> Still check if the vm is currently protected, but without locking and
>> consider it a heuristic.
>>
>> Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> 
> Makes sense to me.
> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
>> ---
>>   arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index ca96f84db2cc..53adbe86a68f 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>>           return -EINVAL;
>>       if (mop->size > MEM_OP_MAX_SIZE)
>>           return -E2BIG;
>> -    if (kvm_s390_pv_is_protected(kvm))
>> +    /*
>> +     * This is technically a heuristic only, if the kvm->lock is not
>> +     * taken, it is not guaranteed that the vm is/remains non-protected.
>> +     * This is ok from a kernel perspective, wrongdoing is detected
>> +     * on the access, -EFAULT is returned and the vm may crash the
>> +     * next time it accesses the memory in question.
>> +     * There is no sane usecase to do switching and a memop on two
>> +     * different CPUs at the same time.
>> +     */
>> +    if (kvm_s390_pv_get_handle(kvm))
>>           return -EINVAL;
>>       if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>>           if (access_key_invalid(mop->key))
>>
>> base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] KVM: s390: Fix lockdep issue in vm memop
  2022-03-23  8:52   ` Janis Schoetterl-Glausch
@ 2022-03-23  8:57     ` Janosch Frank
  2022-03-23  9:30       ` Christian Borntraeger
  0 siblings, 1 reply; 8+ messages in thread
From: Janosch Frank @ 2022-03-23  8:57 UTC (permalink / raw)
  To: Janis Schoetterl-Glausch, Christian Borntraeger,
	Claudio Imbrenda, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev
  Cc: David Hildenbrand, Sven Schnelle, kvm, linux-s390, linux-kernel

On 3/23/22 09:52, Janis Schoetterl-Glausch wrote:
> On 3/23/22 08:58, Janosch Frank wrote:
>> On 3/22/22 16:32, Janis Schoetterl-Glausch wrote:
>>> Issuing a memop on a protected vm does not make sense,
>>
>> Issuing a vm memop on a protected vm...
>>
>> The cpu memop still makes sense, no?
> 
> The vcpu memop does hold the vcpu->lock, so no lockdep issue.
> If you issue a vcpu memop while enabling protected virtualization,
> the memop might find that the vcpu is not protected, while other vcpus
> might already be, but I don't think there's a way to create secure memory
> concurrent with the memop.

I just wanted you to make this a bit more specific since we now have vm 
and vcpu memops. vm memops don't make sense for pv guests but vcpu ones 
are needed to access the sida.

>>
>>> neither is the memory readable/writable, nor does it make sense to check
>>> storage keys. This is why the ioctl will return -EINVAL when it detects
>>> the vm to be protected. However, in order to ensure that the vm cannot
>>> become protected during the memop, the kvm->lock would need to be taken
>>> for the duration of the ioctl. This is also required because
>>> kvm_s390_pv_is_protected asserts that the lock must be held.
>>> Instead, don't try to prevent this. If user space enables secure
>>> execution concurrently with a memop it must accecpt the possibility of
>>> the memop failing.
>>> Still check if the vm is currently protected, but without locking and
>>> consider it a heuristic.
>>>
>>> Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>
>> Makes sense to me.
>>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>
>>> ---
>>>    arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index ca96f84db2cc..53adbe86a68f 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>>>            return -EINVAL;
>>>        if (mop->size > MEM_OP_MAX_SIZE)
>>>            return -E2BIG;
>>> -    if (kvm_s390_pv_is_protected(kvm))
>>> +    /*
>>> +     * This is technically a heuristic only, if the kvm->lock is not
>>> +     * taken, it is not guaranteed that the vm is/remains non-protected.
>>> +     * This is ok from a kernel perspective, wrongdoing is detected
>>> +     * on the access, -EFAULT is returned and the vm may crash the
>>> +     * next time it accesses the memory in question.
>>> +     * There is no sane usecase to do switching and a memop on two
>>> +     * different CPUs at the same time.
>>> +     */
>>> +    if (kvm_s390_pv_get_handle(kvm))
>>>            return -EINVAL;
>>>        if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>>>            if (access_key_invalid(mop->key))
>>>
>>> base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912
>>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] KVM: s390: Fix lockdep issue in vm memop
  2022-03-23  8:57     ` Janosch Frank
@ 2022-03-23  9:30       ` Christian Borntraeger
  2022-03-23  9:39         ` Janis Schoetterl-Glausch
  0 siblings, 1 reply; 8+ messages in thread
From: Christian Borntraeger @ 2022-03-23  9:30 UTC (permalink / raw)
  To: Janosch Frank, Janis Schoetterl-Glausch, Claudio Imbrenda,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev
  Cc: David Hildenbrand, Sven Schnelle, kvm, linux-s390, linux-kernel



Am 23.03.22 um 09:57 schrieb Janosch Frank:
> On 3/23/22 09:52, Janis Schoetterl-Glausch wrote:
>> On 3/23/22 08:58, Janosch Frank wrote:
>>> On 3/22/22 16:32, Janis Schoetterl-Glausch wrote:
>>>> Issuing a memop on a protected vm does not make sense,
>>>
>>> Issuing a vm memop on a protected vm...
>>>
>>> The cpu memop still makes sense, no?
>>
>> The vcpu memop does hold the vcpu->lock, so no lockdep issue.
>> If you issue a vcpu memop while enabling protected virtualization,
>> the memop might find that the vcpu is not protected, while other vcpus
>> might already be, but I don't think there's a way to create secure memory
>> concurrent with the memop.
> 
> I just wanted you to make this a bit more specific since we now have vm and vcpu memops. vm memops don't make sense for pv guests but vcpu ones are needed to access the sida.

Right, I think changing the commit messages
- Issuing a memop on a protected vm does not make sense
+ Issuing a vm memop on a protected vm does not make sense

does make sense.

> 
>>>
>>>> neither is the memory readable/writable, nor does it make sense to check
>>>> storage keys. This is why the ioctl will return -EINVAL when it detects
>>>> the vm to be protected. However, in order to ensure that the vm cannot
>>>> become protected during the memop, the kvm->lock would need to be taken
>>>> for the duration of the ioctl. This is also required because
>>>> kvm_s390_pv_is_protected asserts that the lock must be held.
>>>> Instead, don't try to prevent this. If user space enables secure
>>>> execution concurrently with a memop it must accecpt the possibility of
>>>> the memop failing.
>>>> Still check if the vm is currently protected, but without locking and
>>>> consider it a heuristic.
>>>>
>>>> Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
>>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>>
>>> Makes sense to me.
>>>
>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>
>>>> ---
>>>>    arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
>>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index ca96f84db2cc..53adbe86a68f 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>>>>            return -EINVAL;
>>>>        if (mop->size > MEM_OP_MAX_SIZE)
>>>>            return -E2BIG;
>>>> -    if (kvm_s390_pv_is_protected(kvm))
>>>> +    /*
>>>> +     * This is technically a heuristic only, if the kvm->lock is not
>>>> +     * taken, it is not guaranteed that the vm is/remains non-protected.
>>>> +     * This is ok from a kernel perspective, wrongdoing is detected
>>>> +     * on the access, -EFAULT is returned and the vm may crash the
>>>> +     * next time it accesses the memory in question.
>>>> +     * There is no sane usecase to do switching and a memop on two
>>>> +     * different CPUs at the same time.
>>>> +     */
>>>> +    if (kvm_s390_pv_get_handle(kvm))
>>>>            return -EINVAL;
>>>>        if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>>>>            if (access_key_invalid(mop->key))
>>>>
>>>> base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912
>>>
>>
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] KVM: s390: Fix lockdep issue in vm memop
  2022-03-23  9:30       ` Christian Borntraeger
@ 2022-03-23  9:39         ` Janis Schoetterl-Glausch
  2022-03-23  9:45           ` Christian Borntraeger
  0 siblings, 1 reply; 8+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-03-23  9:39 UTC (permalink / raw)
  To: Christian Borntraeger, Janosch Frank, Claudio Imbrenda,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev
  Cc: David Hildenbrand, Sven Schnelle, kvm, linux-s390, linux-kernel

On 3/23/22 10:30, Christian Borntraeger wrote:
> 
> 
> Am 23.03.22 um 09:57 schrieb Janosch Frank:
>> On 3/23/22 09:52, Janis Schoetterl-Glausch wrote:
>>> On 3/23/22 08:58, Janosch Frank wrote:
>>>> On 3/22/22 16:32, Janis Schoetterl-Glausch wrote:
>>>>> Issuing a memop on a protected vm does not make sense,
>>>>
>>>> Issuing a vm memop on a protected vm...
>>>>
>>>> The cpu memop still makes sense, no?
>>>
>>> The vcpu memop does hold the vcpu->lock, so no lockdep issue.
>>> If you issue a vcpu memop while enabling protected virtualization,
>>> the memop might find that the vcpu is not protected, while other vcpus
>>> might already be, but I don't think there's a way to create secure memory
>>> concurrent with the memop.
>>
>> I just wanted you to make this a bit more specific since we now have vm and vcpu memops. vm memops don't make sense for pv guests but vcpu ones are needed to access the sida.
> 
> Right, I think changing the commit messages
> - Issuing a memop on a protected vm does not make sense
> + Issuing a vm memop on a protected vm does not make sense
> 
> does make sense.

Ok, want me to send a v2?
> 
>>
>>>>
>>>>> neither is the memory readable/writable, nor does it make sense to check
>>>>> storage keys. This is why the ioctl will return -EINVAL when it detects
>>>>> the vm to be protected. However, in order to ensure that the vm cannot
>>>>> become protected during the memop, the kvm->lock would need to be taken
>>>>> for the duration of the ioctl. This is also required because
>>>>> kvm_s390_pv_is_protected asserts that the lock must be held.
>>>>> Instead, don't try to prevent this. If user space enables secure
>>>>> execution concurrently with a memop it must accecpt the possibility of
>>>>> the memop failing.
>>>>> Still check if the vm is currently protected, but without locking and
>>>>> consider it a heuristic.
>>>>>
>>>>> Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
>>>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>>>
>>>> Makes sense to me.
>>>>
>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>>
>>>>> ---
>>>>>    arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
>>>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>> index ca96f84db2cc..53adbe86a68f 100644
>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>> @@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>>>>>            return -EINVAL;
>>>>>        if (mop->size > MEM_OP_MAX_SIZE)
>>>>>            return -E2BIG;
>>>>> -    if (kvm_s390_pv_is_protected(kvm))
>>>>> +    /*
>>>>> +     * This is technically a heuristic only, if the kvm->lock is not
>>>>> +     * taken, it is not guaranteed that the vm is/remains non-protected.
>>>>> +     * This is ok from a kernel perspective, wrongdoing is detected
>>>>> +     * on the access, -EFAULT is returned and the vm may crash the
>>>>> +     * next time it accesses the memory in question.
>>>>> +     * There is no sane usecase to do switching and a memop on two
>>>>> +     * different CPUs at the same time.
>>>>> +     */
>>>>> +    if (kvm_s390_pv_get_handle(kvm))
>>>>>            return -EINVAL;
>>>>>        if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>>>>>            if (access_key_invalid(mop->key))
>>>>>
>>>>> base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912
>>>>
>>>
>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] KVM: s390: Fix lockdep issue in vm memop
  2022-03-23  9:39         ` Janis Schoetterl-Glausch
@ 2022-03-23  9:45           ` Christian Borntraeger
  0 siblings, 0 replies; 8+ messages in thread
From: Christian Borntraeger @ 2022-03-23  9:45 UTC (permalink / raw)
  To: Janis Schoetterl-Glausch, Janosch Frank, Claudio Imbrenda,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev
  Cc: David Hildenbrand, Sven Schnelle, kvm, linux-s390, linux-kernel



Am 23.03.22 um 10:39 schrieb Janis Schoetterl-Glausch:
> On 3/23/22 10:30, Christian Borntraeger wrote:
>>
>>
>> Am 23.03.22 um 09:57 schrieb Janosch Frank:
>>> On 3/23/22 09:52, Janis Schoetterl-Glausch wrote:
>>>> On 3/23/22 08:58, Janosch Frank wrote:
>>>>> On 3/22/22 16:32, Janis Schoetterl-Glausch wrote:
>>>>>> Issuing a memop on a protected vm does not make sense,
>>>>>
>>>>> Issuing a vm memop on a protected vm...
>>>>>
>>>>> The cpu memop still makes sense, no?
>>>>
>>>> The vcpu memop does hold the vcpu->lock, so no lockdep issue.
>>>> If you issue a vcpu memop while enabling protected virtualization,
>>>> the memop might find that the vcpu is not protected, while other vcpus
>>>> might already be, but I don't think there's a way to create secure memory
>>>> concurrent with the memop.
>>>
>>> I just wanted you to make this a bit more specific since we now have vm and vcpu memops. vm memops don't make sense for pv guests but vcpu ones are needed to access the sida.
>>
>> Right, I think changing the commit messages
>> - Issuing a memop on a protected vm does not make sense
>> + Issuing a vm memop on a protected vm does not make sense
>>
>> does make sense.
> 
> Ok, want me to send a v2?

I can fixup when applying. Done and queued for kvm.
>>
>>>
>>>>>
>>>>>> neither is the memory readable/writable, nor does it make sense to check
>>>>>> storage keys. This is why the ioctl will return -EINVAL when it detects
>>>>>> the vm to be protected. However, in order to ensure that the vm cannot
>>>>>> become protected during the memop, the kvm->lock would need to be taken
>>>>>> for the duration of the ioctl. This is also required because
>>>>>> kvm_s390_pv_is_protected asserts that the lock must be held.
>>>>>> Instead, don't try to prevent this. If user space enables secure
>>>>>> execution concurrently with a memop it must accecpt the possibility of
>>>>>> the memop failing.
>>>>>> Still check if the vm is currently protected, but without locking and
>>>>>> consider it a heuristic.
>>>>>>
>>>>>> Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
>>>>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>>>>
>>>>> Makes sense to me.
>>>>>
>>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>
>>>>>> ---
>>>>>>     arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
>>>>>>     1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>>> index ca96f84db2cc..53adbe86a68f 100644
>>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>>> @@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>>>>>>             return -EINVAL;
>>>>>>         if (mop->size > MEM_OP_MAX_SIZE)
>>>>>>             return -E2BIG;
>>>>>> -    if (kvm_s390_pv_is_protected(kvm))
>>>>>> +    /*
>>>>>> +     * This is technically a heuristic only, if the kvm->lock is not
>>>>>> +     * taken, it is not guaranteed that the vm is/remains non-protected.
>>>>>> +     * This is ok from a kernel perspective, wrongdoing is detected
>>>>>> +     * on the access, -EFAULT is returned and the vm may crash the
>>>>>> +     * next time it accesses the memory in question.
>>>>>> +     * There is no sane usecase to do switching and a memop on two
>>>>>> +     * different CPUs at the same time.
>>>>>> +     */
>>>>>> +    if (kvm_s390_pv_get_handle(kvm))
>>>>>>             return -EINVAL;
>>>>>>         if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>>>>>>             if (access_key_invalid(mop->key))
>>>>>>
>>>>>> base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912
>>>>>
>>>>
>>>
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-03-23  9:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-22 15:32 [PATCH] KVM: s390: Fix lockdep issue in vm memop Janis Schoetterl-Glausch
2022-03-22 16:15 ` Claudio Imbrenda
2022-03-23  7:58 ` Janosch Frank
2022-03-23  8:52   ` Janis Schoetterl-Glausch
2022-03-23  8:57     ` Janosch Frank
2022-03-23  9:30       ` Christian Borntraeger
2022-03-23  9:39         ` Janis Schoetterl-Glausch
2022-03-23  9:45           ` Christian Borntraeger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.