[intel-sgx-kernel-dev,3/4] intel_sgx: combine epc_page and va_page into union
diff mbox

Message ID 1492009821-22428-4-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson April 12, 2017, 3:10 p.m. UTC
Combine sgx_encl_page's epc_page and va_page into a union.  On-demand
VA slot/page allocations guarantees only one of epc_page or va_page
will be valid at any given time.

Add a new flag, SGX_ENCL_PAGE_EPC_VALID, to track if an encl page has
been loaded into the EPC.  If SGX_ENCL_PAGE_EPC_VALID is set, then the
epc_page member of the epc_page/va_page union is valid, otherwise
va_page *may* be valid, e.g. neither is valid if EADD has not been
completed.

Move the functionality of sgx_evict_page into the success path of
EWB, as the EPC page must be freed before updating the encl_page's
va_page.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 drivers/platform/x86/intel_sgx/sgx.h            |  9 ++++++---
 drivers/platform/x86/intel_sgx/sgx_ioctl.c      |  2 ++
 drivers/platform/x86/intel_sgx/sgx_page_cache.c | 11 ++++++-----
 drivers/platform/x86/intel_sgx/sgx_util.c       | 11 +++++------
 4 files changed, 19 insertions(+), 14 deletions(-)

Comments

Jarkko Sakkinen April 13, 2017, 1:09 p.m. UTC | #1
On Wed, Apr 12, 2017 at 08:10:20AM -0700, Sean Christopherson wrote:
> Combine sgx_encl_page's epc_page and va_page into a union.  On-demand
> VA slot/page allocations guarantees only one of epc_page or va_page
> will be valid at any given time.
> 
> Add a new flag, SGX_ENCL_PAGE_EPC_VALID, to track if an encl page has
> been loaded into the EPC.  If SGX_ENCL_PAGE_EPC_VALID is set, then the
> epc_page member of the epc_page/va_page union is valid, otherwise
> va_page *may* be valid, e.g. neither is valid if EADD has not been
> completed.
> 
> Move the functionality of sgx_evict_page into the success path of
> EWB, as the EPC page must be freed before updating the encl_page's
> va_page.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

This is that I proposed in the Fall when we discussed about what needs
to be done and it has been sitting in my backlog. Thanks for fixing
this issue!

/Jarkko

> ---
>  drivers/platform/x86/intel_sgx/sgx.h            |  9 ++++++---
>  drivers/platform/x86/intel_sgx/sgx_ioctl.c      |  2 ++
>  drivers/platform/x86/intel_sgx/sgx_page_cache.c | 11 ++++++-----
>  drivers/platform/x86/intel_sgx/sgx_util.c       | 11 +++++------
>  4 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
> index 633526f..a1f59dd 100644
> --- a/drivers/platform/x86/intel_sgx/sgx.h
> +++ b/drivers/platform/x86/intel_sgx/sgx.h
> @@ -91,15 +91,18 @@ static inline void sgx_free_va_slot(struct sgx_va_page *page,
>  enum sgx_encl_page_flags {
>  	SGX_ENCL_PAGE_TCS	= BIT(0),
>  	SGX_ENCL_PAGE_RESERVED	= BIT(1),
> +	SGX_ENCL_PAGE_EPC_VALID	= BIT(2),
>  };
>  
>  struct sgx_encl_page {
>  	unsigned long addr;
>  	unsigned int flags;
> -	struct sgx_epc_page *epc_page;
> -	struct list_head load_list;
> -	struct sgx_va_page *va_page;
>  	unsigned int va_offset;
> +	union {
> +		struct sgx_epc_page *epc_page;
> +		struct sgx_va_page *va_page;
> +	};
> +	struct list_head load_list;
>  };
>  
>  struct sgx_tgid_ctx {
> diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> index af80571..6719e56 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> @@ -267,6 +267,7 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
>  		goto out;
>  	}
>  
> +	encl_page->flags |= SGX_ENCL_PAGE_EPC_VALID;
>  	encl_page->epc_page = epc_page;
>  	sgx_test_and_clear_young(encl_page, encl);
>  	list_add_tail(&encl_page->load_list, &encl->load_list);
> @@ -558,6 +559,7 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd,
>  		goto out;
>  	}
>  
> +	encl->secs_page.flags |= SGX_ENCL_PAGE_EPC_VALID;
>  	encl->secs_page.epc_page = secs_epc;
>  	createp->src = (unsigned long)encl->base;
>  
> diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> index a2e39a1..346b82c 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> @@ -341,10 +341,14 @@ static int __sgx_ewb(struct sgx_encl *encl,
>  	sgx_put_page(va);
>  	sgx_put_page(epc);
>  
> -	if (ret == SGX_SUCCESS)
> +	if (ret == SGX_SUCCESS) {
> +		encl_page->flags &= ~(SGX_ENCL_PAGE_RESERVED | SGX_ENCL_PAGE_EPC_VALID);
> +		sgx_free_page(encl_page->epc_page, encl);
>  		encl_page->va_page = va_page;
> -	else
> +	}
> +	else {
>  		sgx_free_va_slot(va_page, encl_page->va_offset);
> +	}
>  
>  out_pcmd:
>  	sgx_put_backing(pcmd, true);
> @@ -380,9 +384,6 @@ static void sgx_evict_page(struct sgx_encl_page *entry,
>  			   struct sgx_encl *encl)
>  {
>  	sgx_ewb(encl, entry);
> -	sgx_free_page(entry->epc_page, encl);
> -	entry->epc_page = NULL;
> -	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
>  }
>  
>  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
> diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
> index f4b2f1a..f3c8b39 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_util.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_util.c
> @@ -220,6 +220,7 @@ static int sgx_eldu(struct sgx_encl *encl,
>  		ret = -EFAULT;
>  	} else {
>  		sgx_free_va_slot(encl_page->va_page, encl_page->va_offset);
> +		encl_page->flags |= SGX_ENCL_PAGE_EPC_VALID;
>  		encl_page->epc_page = epc_page;
>  	}
>  
> @@ -281,7 +282,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  	}
>  
>  	/* Legal race condition, page is already faulted. */
> -	if (entry->epc_page) {
> +	if (entry->flags & SGX_ENCL_PAGE_EPC_VALID) {
>  		if (reserve)
>  			entry->flags |= SGX_ENCL_PAGE_RESERVED;
>  		goto out;
> @@ -295,7 +296,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  	}
>  
>  	/* If SECS is evicted then reload it first */
> -	if (!encl->secs_page.epc_page) {
> +	if (!(encl->secs_page.flags & SGX_ENCL_PAGE_EPC_VALID)) {
>  		secs_epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC);
>  		if (IS_ERR(secs_epc_page)) {
>  			rc = PTR_ERR(secs_epc_page);
> @@ -374,7 +375,7 @@ void sgx_encl_release(struct kref *ref)
>  
>  	radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) {
>  		entry = *slot;
> -		if (entry->epc_page) {
> +		if (entry->flags & SGX_ENCL_PAGE_EPC_VALID) {
>  			list_del(&entry->load_list);
>  			sgx_free_page(entry->epc_page, encl);
>  		}
> @@ -390,11 +391,9 @@ void sgx_encl_release(struct kref *ref)
>  		kfree(va_page);
>  	}
>  
> -	if (encl->secs_page.epc_page)
> +	if (encl->secs_page.flags & SGX_ENCL_PAGE_EPC_VALID)
>  		sgx_free_page(encl->secs_page.epc_page, encl);
>  
> -	encl->secs_page.epc_page = NULL;
> -
>  	if (encl->tgid_ctx)
>  		kref_put(&encl->tgid_ctx->refcount, sgx_tgid_ctx_release);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
index 633526f..a1f59dd 100644
--- a/drivers/platform/x86/intel_sgx/sgx.h
+++ b/drivers/platform/x86/intel_sgx/sgx.h
@@ -91,15 +91,18 @@  static inline void sgx_free_va_slot(struct sgx_va_page *page,
 enum sgx_encl_page_flags {
 	SGX_ENCL_PAGE_TCS	= BIT(0),
 	SGX_ENCL_PAGE_RESERVED	= BIT(1),
+	SGX_ENCL_PAGE_EPC_VALID	= BIT(2),
 };
 
 struct sgx_encl_page {
 	unsigned long addr;
 	unsigned int flags;
-	struct sgx_epc_page *epc_page;
-	struct list_head load_list;
-	struct sgx_va_page *va_page;
 	unsigned int va_offset;
+	union {
+		struct sgx_epc_page *epc_page;
+		struct sgx_va_page *va_page;
+	};
+	struct list_head load_list;
 };
 
 struct sgx_tgid_ctx {
diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
index af80571..6719e56 100644
--- a/drivers/platform/x86/intel_sgx/sgx_ioctl.c
+++ b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
@@ -267,6 +267,7 @@  static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
 		goto out;
 	}
 
+	encl_page->flags |= SGX_ENCL_PAGE_EPC_VALID;
 	encl_page->epc_page = epc_page;
 	sgx_test_and_clear_young(encl_page, encl);
 	list_add_tail(&encl_page->load_list, &encl->load_list);
@@ -558,6 +559,7 @@  static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd,
 		goto out;
 	}
 
+	encl->secs_page.flags |= SGX_ENCL_PAGE_EPC_VALID;
 	encl->secs_page.epc_page = secs_epc;
 	createp->src = (unsigned long)encl->base;
 
diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
index a2e39a1..346b82c 100644
--- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
@@ -341,10 +341,14 @@  static int __sgx_ewb(struct sgx_encl *encl,
 	sgx_put_page(va);
 	sgx_put_page(epc);
 
-	if (ret == SGX_SUCCESS)
+	if (ret == SGX_SUCCESS) {
+		encl_page->flags &= ~(SGX_ENCL_PAGE_RESERVED | SGX_ENCL_PAGE_EPC_VALID);
+		sgx_free_page(encl_page->epc_page, encl);
 		encl_page->va_page = va_page;
-	else
+	}
+	else {
 		sgx_free_va_slot(va_page, encl_page->va_offset);
+	}
 
 out_pcmd:
 	sgx_put_backing(pcmd, true);
@@ -380,9 +384,6 @@  static void sgx_evict_page(struct sgx_encl_page *entry,
 			   struct sgx_encl *encl)
 {
 	sgx_ewb(encl, entry);
-	sgx_free_page(entry->epc_page, encl);
-	entry->epc_page = NULL;
-	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
 }
 
 static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
index f4b2f1a..f3c8b39 100644
--- a/drivers/platform/x86/intel_sgx/sgx_util.c
+++ b/drivers/platform/x86/intel_sgx/sgx_util.c
@@ -220,6 +220,7 @@  static int sgx_eldu(struct sgx_encl *encl,
 		ret = -EFAULT;
 	} else {
 		sgx_free_va_slot(encl_page->va_page, encl_page->va_offset);
+		encl_page->flags |= SGX_ENCL_PAGE_EPC_VALID;
 		encl_page->epc_page = epc_page;
 	}
 
@@ -281,7 +282,7 @@  static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
 	}
 
 	/* Legal race condition, page is already faulted. */
-	if (entry->epc_page) {
+	if (entry->flags & SGX_ENCL_PAGE_EPC_VALID) {
 		if (reserve)
 			entry->flags |= SGX_ENCL_PAGE_RESERVED;
 		goto out;
@@ -295,7 +296,7 @@  static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
 	}
 
 	/* If SECS is evicted then reload it first */
-	if (!encl->secs_page.epc_page) {
+	if (!(encl->secs_page.flags & SGX_ENCL_PAGE_EPC_VALID)) {
 		secs_epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC);
 		if (IS_ERR(secs_epc_page)) {
 			rc = PTR_ERR(secs_epc_page);
@@ -374,7 +375,7 @@  void sgx_encl_release(struct kref *ref)
 
 	radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) {
 		entry = *slot;
-		if (entry->epc_page) {
+		if (entry->flags & SGX_ENCL_PAGE_EPC_VALID) {
 			list_del(&entry->load_list);
 			sgx_free_page(entry->epc_page, encl);
 		}
@@ -390,11 +391,9 @@  void sgx_encl_release(struct kref *ref)
 		kfree(va_page);
 	}
 
-	if (encl->secs_page.epc_page)
+	if (encl->secs_page.flags & SGX_ENCL_PAGE_EPC_VALID)
 		sgx_free_page(encl->secs_page.epc_page, encl);
 
-	encl->secs_page.epc_page = NULL;
-
 	if (encl->tgid_ctx)
 		kref_put(&encl->tgid_ctx->refcount, sgx_tgid_ctx_release);