[RFC,v2] KVM: s390: protvirt: Secure memory is not mergeable
diff mbox series

Message ID 20191025082446.754-1-frankja@linux.ibm.com
State New
Headers show
Series
  • [RFC,v2] KVM: s390: protvirt: Secure memory is not mergeable
Related show

Commit Message

Janosch Frank Oct. 25, 2019, 8:24 a.m. UTC
KSM will not work on secure pages, because when the kernel reads a
secure page, it will be encrypted and hence no two pages will look the
same.

Let's mark the guest pages as unmergeable when we transition to secure
mode.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/include/asm/gmap.h |  1 +
 arch/s390/kvm/kvm-s390.c     |  6 ++++++
 arch/s390/mm/gmap.c          | 32 +++++++++++++++++++++-----------
 3 files changed, 28 insertions(+), 11 deletions(-)

Comments

Christian Borntraeger Nov. 1, 2019, 1:02 p.m. UTC | #1
On 25.10.19 10:24, Janosch Frank wrote:
> KSM will not work on secure pages, because when the kernel reads a
> secure page, it will be encrypted and hence no two pages will look the
> same.
> 
> Let's mark the guest pages as unmergeable when we transition to secure
> mode.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

v2 Looks better.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  arch/s390/include/asm/gmap.h |  1 +
>  arch/s390/kvm/kvm-s390.c     |  6 ++++++
>  arch/s390/mm/gmap.c          | 32 +++++++++++++++++++++-----------
>  3 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
> index 6efc0b501227..eab6a2ec3599 100644
> --- a/arch/s390/include/asm/gmap.h
> +++ b/arch/s390/include/asm/gmap.h
> @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *, unsigned long start,
>  
>  void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
>  			     unsigned long gaddr, unsigned long vmaddr);
> +int gmap_mark_unmergeable(void);
>  #endif /* _ASM_S390_GMAP_H */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 924132d92782..d1ba12f857e7 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>  		if (r)
>  			break;
>  
> +		down_write(&current->mm->mmap_sem);
> +		r = gmap_mark_unmergeable();
> +		up_write(&current->mm->mmap_sem);
> +		if (r)
> +			break;
> +
>  		mutex_lock(&kvm->lock);
>  		kvm_s390_vcpu_block_all(kvm);
>  		/* FMT 4 SIE needs esca */
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index edcdca97e85e..faecdf81abdb 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>  }
>  EXPORT_SYMBOL_GPL(s390_enable_sie);
>  
> +int gmap_mark_unmergeable(void)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +
> +
> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> +				MADV_UNMERGEABLE, &vma->vm_flags))
> +			return -ENOMEM;
> +	}
> +	mm->def_flags &= ~VM_MERGEABLE;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
> +
>  /*
>   * Enable storage key handling from now on and initialize the storage
>   * keys with the default key.
> @@ -2593,24 +2610,17 @@ static const struct mm_walk_ops enable_skey_walk_ops = {
>  int s390_enable_skey(void)
>  {
>  	struct mm_struct *mm = current->mm;
> -	struct vm_area_struct *vma;
>  	int rc = 0;
>  
>  	down_write(&mm->mmap_sem);
>  	if (mm_uses_skeys(mm))
>  		goto out_up;
>  
> -	mm->context.uses_skeys = 1;
> -	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> -		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> -				MADV_UNMERGEABLE, &vma->vm_flags)) {
> -			mm->context.uses_skeys = 0;
> -			rc = -ENOMEM;
> -			goto out_up;
> -		}
> -	}
> -	mm->def_flags &= ~VM_MERGEABLE;
> +	rc = gmap_mark_unmergeable();
> +	if (rc)
> +		goto out_up;
>  
> +	mm->context.uses_skeys = 1;
>  	walk_page_range(mm, 0, TASK_SIZE, &enable_skey_walk_ops, NULL);
>  
>  out_up:
>
David Hildenbrand Nov. 4, 2019, 2:32 p.m. UTC | #2
On 25.10.19 10:24, Janosch Frank wrote:
> KSM will not work on secure pages, because when the kernel reads a
> secure page, it will be encrypted and hence no two pages will look the
> same.
> 
> Let's mark the guest pages as unmergeable when we transition to secure
> mode.

Patch itself looks good to me, but I do wonder: Is this really needed 
when pinning all encrypted pages currently?

Not sure about races between KSM and the pinning/encrypting thread, 
similar to paging, though ...
Janosch Frank Nov. 4, 2019, 2:36 p.m. UTC | #3
On 11/4/19 3:32 PM, David Hildenbrand wrote:
> On 25.10.19 10:24, Janosch Frank wrote:
>> KSM will not work on secure pages, because when the kernel reads a
>> secure page, it will be encrypted and hence no two pages will look the
>> same.
>>
>> Let's mark the guest pages as unmergeable when we transition to secure
>> mode.
> 
> Patch itself looks good to me, but I do wonder: Is this really needed 
> when pinning all encrypted pages currently?
> 
> Not sure about races between KSM and the pinning/encrypting thread, 
> similar to paging, though ...
> 

The pinning was added several months after I wrote the patch.
Now that we have it, we really need to have another proper look at the
whole topic.

Thanks for your review :-)
David Hildenbrand Nov. 4, 2019, 2:38 p.m. UTC | #4
On 04.11.19 15:36, Janosch Frank wrote:
> On 11/4/19 3:32 PM, David Hildenbrand wrote:
>> On 25.10.19 10:24, Janosch Frank wrote:
>>> KSM will not work on secure pages, because when the kernel reads a
>>> secure page, it will be encrypted and hence no two pages will look the
>>> same.
>>>
>>> Let's mark the guest pages as unmergeable when we transition to secure
>>> mode.
>>
>> Patch itself looks good to me, but I do wonder: Is this really needed
>> when pinning all encrypted pages currently?
>>
>> Not sure about races between KSM and the pinning/encrypting thread,
>> similar to paging, though ...
>>
> 
> The pinning was added several months after I wrote the patch.
> Now that we have it, we really need to have another proper look at the
> whole topic.
> 
> Thanks for your review :-)

I'd certainly prefer this patch (+some way to mlock) over pinning ;)

You can have

Reviewed-by: David Hildenbrand <david@redhat.com>

For this patch, if you end up needing it :)
Thomas Huth Nov. 13, 2019, 12:23 p.m. UTC | #5
On 25/10/2019 10.24, Janosch Frank wrote:
> KSM will not work on secure pages, because when the kernel reads a
> secure page, it will be encrypted and hence no two pages will look the
> same.
> 
> Let's mark the guest pages as unmergeable when we transition to secure
> mode.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
[...]
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index edcdca97e85e..faecdf81abdb 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>  }
>  EXPORT_SYMBOL_GPL(s390_enable_sie);
>  
> +int gmap_mark_unmergeable(void)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +
> +

Please remove one of the two empty lines.

> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> +				MADV_UNMERGEABLE, &vma->vm_flags))
> +			return -ENOMEM;
> +	}
> +	mm->def_flags &= ~VM_MERGEABLE;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
> +
[...]

Apart from the cosmetic nit, the patch looks fine to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Janosch Frank Nov. 13, 2019, 3:54 p.m. UTC | #6
On 11/13/19 1:23 PM, Thomas Huth wrote:
> On 25/10/2019 10.24, Janosch Frank wrote:
>> KSM will not work on secure pages, because when the kernel reads a
>> secure page, it will be encrypted and hence no two pages will look the
>> same.
>>
>> Let's mark the guest pages as unmergeable when we transition to secure
>> mode.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
> [...]
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index edcdca97e85e..faecdf81abdb 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>>  }
>>  EXPORT_SYMBOL_GPL(s390_enable_sie);
>>  
>> +int gmap_mark_unmergeable(void)
>> +{
>> +	struct mm_struct *mm = current->mm;
>> +	struct vm_area_struct *vma;
>> +
>> +
> 
> Please remove one of the two empty lines.

Already gone

> 
>> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
>> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
>> +				MADV_UNMERGEABLE, &vma->vm_flags))
>> +			return -ENOMEM;
>> +	}
>> +	mm->def_flags &= ~VM_MERGEABLE;
>> +

Also this one was removed

>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
>> +
> [...]
> 
> Apart from the cosmetic nit, the patch looks fine to me.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
Thanks!

Patch
diff mbox series

diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
index 6efc0b501227..eab6a2ec3599 100644
--- a/arch/s390/include/asm/gmap.h
+++ b/arch/s390/include/asm/gmap.h
@@ -145,4 +145,5 @@  int gmap_mprotect_notify(struct gmap *, unsigned long start,
 
 void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
 			     unsigned long gaddr, unsigned long vmaddr);
+int gmap_mark_unmergeable(void);
 #endif /* _ASM_S390_GMAP_H */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 924132d92782..d1ba12f857e7 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2176,6 +2176,12 @@  static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
 		if (r)
 			break;
 
+		down_write(&current->mm->mmap_sem);
+		r = gmap_mark_unmergeable();
+		up_write(&current->mm->mmap_sem);
+		if (r)
+			break;
+
 		mutex_lock(&kvm->lock);
 		kvm_s390_vcpu_block_all(kvm);
 		/* FMT 4 SIE needs esca */
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index edcdca97e85e..faecdf81abdb 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2548,6 +2548,23 @@  int s390_enable_sie(void)
 }
 EXPORT_SYMBOL_GPL(s390_enable_sie);
 
+int gmap_mark_unmergeable(void)
+{
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *vma;
+
+
+	for (vma = mm->mmap; vma; vma = vma->vm_next) {
+		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
+				MADV_UNMERGEABLE, &vma->vm_flags))
+			return -ENOMEM;
+	}
+	mm->def_flags &= ~VM_MERGEABLE;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
+
 /*
  * Enable storage key handling from now on and initialize the storage
  * keys with the default key.
@@ -2593,24 +2610,17 @@  static const struct mm_walk_ops enable_skey_walk_ops = {
 int s390_enable_skey(void)
 {
 	struct mm_struct *mm = current->mm;
-	struct vm_area_struct *vma;
 	int rc = 0;
 
 	down_write(&mm->mmap_sem);
 	if (mm_uses_skeys(mm))
 		goto out_up;
 
-	mm->context.uses_skeys = 1;
-	for (vma = mm->mmap; vma; vma = vma->vm_next) {
-		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
-				MADV_UNMERGEABLE, &vma->vm_flags)) {
-			mm->context.uses_skeys = 0;
-			rc = -ENOMEM;
-			goto out_up;
-		}
-	}
-	mm->def_flags &= ~VM_MERGEABLE;
+	rc = gmap_mark_unmergeable();
+	if (rc)
+		goto out_up;
 
+	mm->context.uses_skeys = 1;
 	walk_page_range(mm, 0, TASK_SIZE, &enable_skey_walk_ops, NULL);
 
 out_up: