[intel-sgx-kernel-dev] intel_sgx: use VM_DONTCOPY
diff mbox

Message ID 20170116141918.17382-1-jarkko.sakkinen@linux.intel.com
State New
Headers show

Commit Message

Jarkko Sakkinen Jan. 16, 2017, 2:19 p.m. UTC
In order to have well defined semantics for forking move into using
VM_DONTCOPY for the enclave VMA.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/platform/x86/intel_sgx_main.c |  7 ++-----
 drivers/platform/x86/intel_sgx_vma.c  | 27 +++++----------------------
 2 files changed, 7 insertions(+), 27 deletions(-)

Comments

Jarkko Sakkinen Jan. 16, 2017, 2:43 p.m. UTC | #1
On Mon, Jan 16, 2017 at 04:19:18PM +0200, Jarkko Sakkinen wrote:
> In order to have well defined semantics for forking move into using
> VM_DONTCOPY for the enclave VMA.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/platform/x86/intel_sgx_main.c |  7 ++-----
>  drivers/platform/x86/intel_sgx_vma.c  | 27 +++++----------------------
>  2 files changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c
> index 93dd708..b5e414a 100644
> --- a/drivers/platform/x86/intel_sgx_main.c
> +++ b/drivers/platform/x86/intel_sgx_main.c
> @@ -103,11 +103,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>  static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	vma->vm_ops = &sgx_vm_ops;
> -#if !defined(VM_RESERVED)
> -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
> -#else
> -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO;
> -#endif
> +	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
> +			 VM_DONTCOPY;
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
> index e670405..11d1c3b 100644
> --- a/drivers/platform/x86/intel_sgx_vma.c
> +++ b/drivers/platform/x86/intel_sgx_vma.c
> @@ -71,34 +71,18 @@
>  static void sgx_vma_open(struct vm_area_struct *vma)
>  {
>  	struct sgx_encl *encl = vma->vm_private_data;
> -
> -	/* When forking for the second time vm_private_data is already set to
> -	 * NULL.
> -	 */
> -	if (!encl) {
> -		zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
> +	if (!encl)
>  		return;
> -	}
> -
> -	/* Invalidate enclave when the process has been forked. */
> -	mutex_lock(&encl->lock);
> -	if (encl->mm != vma->vm_mm) {
> -		sgx_invalidate(encl);
> -		vma->vm_private_data = NULL;
> -	}
> -	mutex_unlock(&encl->lock);
>  
> -	if (vma->vm_private_data)
> -		kref_get(&encl->refcount);
> +	/* kref cannot underflow because ECREATE ioctl checks that there is only
> +	 * one single VMA for the enclave before proceeding.
> +	 */

This is not entirely correctly stated. It should say that nullity check
prevents the underflow and the check in ECREATE ensures that kref count
is always in sync.

/Jarkko
Jarkko Sakkinen Jan. 17, 2017, 6:07 p.m. UTC | #2
On Mon, Jan 16, 2017 at 04:19:18PM +0200, Jarkko Sakkinen wrote:
> In order to have well defined semantics for forking move into using
> VM_DONTCOPY for the enclave VMA.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reported-by: Jethro Beekman <jethro@fortanix.com>

Jethro, does this address your problem or not?

/Jarkko


> ---
>  drivers/platform/x86/intel_sgx_main.c |  7 ++-----
>  drivers/platform/x86/intel_sgx_vma.c  | 27 +++++----------------------
>  2 files changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c
> index 93dd708..b5e414a 100644
> --- a/drivers/platform/x86/intel_sgx_main.c
> +++ b/drivers/platform/x86/intel_sgx_main.c
> @@ -103,11 +103,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>  static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	vma->vm_ops = &sgx_vm_ops;
> -#if !defined(VM_RESERVED)
> -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
> -#else
> -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO;
> -#endif
> +	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
> +			 VM_DONTCOPY;
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
> index e670405..11d1c3b 100644
> --- a/drivers/platform/x86/intel_sgx_vma.c
> +++ b/drivers/platform/x86/intel_sgx_vma.c
> @@ -71,34 +71,18 @@
>  static void sgx_vma_open(struct vm_area_struct *vma)
>  {
>  	struct sgx_encl *encl = vma->vm_private_data;
> -
> -	/* When forking for the second time vm_private_data is already set to
> -	 * NULL.
> -	 */
> -	if (!encl) {
> -		zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
> +	if (!encl)
>  		return;
> -	}
> -
> -	/* Invalidate enclave when the process has been forked. */
> -	mutex_lock(&encl->lock);
> -	if (encl->mm != vma->vm_mm) {
> -		sgx_invalidate(encl);
> -		vma->vm_private_data = NULL;
> -	}
> -	mutex_unlock(&encl->lock);
>  
> -	if (vma->vm_private_data)
> -		kref_get(&encl->refcount);
> +	/* kref cannot underflow because ECREATE ioctl checks that there is only
> +	 * one single VMA for the enclave before proceeding.
> +	 */
> +	kref_get(&encl->refcount);
>  }
>  
>  static void sgx_vma_close(struct vm_area_struct *vma)
>  {
>  	struct sgx_encl *encl = vma->vm_private_data;
> -
> -	/* When forking for the second time vm_private_data is already set to
> -	 * NULL.
> -	 */
>  	if (!encl)
>  		return;
>  
> @@ -106,7 +90,6 @@ static void sgx_vma_close(struct vm_area_struct *vma)
>  	zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
>  	encl->flags |= SGX_ENCL_DEAD;
>  	mutex_unlock(&encl->lock);
> -
>  	kref_put(&encl->refcount, sgx_encl_release);
>  }
>  
> -- 
> 2.9.3
>
Jethro Beekman Jan. 20, 2017, 7:01 a.m. UTC | #3
On 2017-01-17 10:07, Jarkko Sakkinen wrote:
> On Mon, Jan 16, 2017 at 04:19:18PM +0200, Jarkko Sakkinen wrote:
>> In order to have well defined semantics for forking move into using
>> VM_DONTCOPY for the enclave VMA.
>>
>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> Reported-by: Jethro Beekman <jethro@fortanix.com>
>
> Jethro, does this address your problem or not?

That seems to be the case yes, thanks.

Jethro
Jarkko Sakkinen Jan. 21, 2017, 9:35 p.m. UTC | #4
On Thu, Jan 19, 2017 at 11:01:32PM -0800, Jethro Beekman wrote:
> On 2017-01-17 10:07, Jarkko Sakkinen wrote:
> > On Mon, Jan 16, 2017 at 04:19:18PM +0200, Jarkko Sakkinen wrote:
> > > In order to have well defined semantics for forking move into using
> > > VM_DONTCOPY for the enclave VMA.
> > > 
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > Reported-by: Jethro Beekman <jethro@fortanix.com>
> > 
> > Jethro, does this address your problem or not?
> 
> That seems to be the case yes, thanks.
> 
> Jethro

OK, I will apply the patch with your Tested/Reviewed-by.

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c
index 93dd708..b5e414a 100644
--- a/drivers/platform/x86/intel_sgx_main.c
+++ b/drivers/platform/x86/intel_sgx_main.c
@@ -103,11 +103,8 @@  long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	vma->vm_ops = &sgx_vm_ops;
-#if !defined(VM_RESERVED)
-	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
-#else
-	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO;
-#endif
+	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
+			 VM_DONTCOPY;
 
 	return 0;
 }
diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
index e670405..11d1c3b 100644
--- a/drivers/platform/x86/intel_sgx_vma.c
+++ b/drivers/platform/x86/intel_sgx_vma.c
@@ -71,34 +71,18 @@ 
 static void sgx_vma_open(struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = vma->vm_private_data;
-
-	/* When forking for the second time vm_private_data is already set to
-	 * NULL.
-	 */
-	if (!encl) {
-		zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
+	if (!encl)
 		return;
-	}
-
-	/* Invalidate enclave when the process has been forked. */
-	mutex_lock(&encl->lock);
-	if (encl->mm != vma->vm_mm) {
-		sgx_invalidate(encl);
-		vma->vm_private_data = NULL;
-	}
-	mutex_unlock(&encl->lock);
 
-	if (vma->vm_private_data)
-		kref_get(&encl->refcount);
+	/* kref cannot underflow because ECREATE ioctl checks that there is only
+	 * one single VMA for the enclave before proceeding.
+	 */
+	kref_get(&encl->refcount);
 }
 
 static void sgx_vma_close(struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = vma->vm_private_data;
-
-	/* When forking for the second time vm_private_data is already set to
-	 * NULL.
-	 */
 	if (!encl)
 		return;
 
@@ -106,7 +90,6 @@  static void sgx_vma_close(struct vm_area_struct *vma)
 	zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
 	encl->flags |= SGX_ENCL_DEAD;
 	mutex_unlock(&encl->lock);
-
 	kref_put(&encl->refcount, sgx_encl_release);
 }