[intel-sgx-kernel-dev,4/6] intel_sgx: Clean-up page freeing in sgx_ewb flow
diff mbox

Message ID 1482437735-4722-5-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson Dec. 22, 2016, 8:15 p.m. UTC
Call sgx_free_encl_page directly from sgx_ewb to make it more clear
why sgx_free_encl_page is called with(out) SGX_FREE_SKIP_EREMOVE.
This removes the need to return success/failure from sgx_ewb, and
eliminates the associated tracking in sgx_write_pages.

Do not invalidate the enclave if it is already dead.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 drivers/platform/x86/intel_sgx_page_cache.c | 65 +++++++++++++----------------
 1 file changed, 29 insertions(+), 36 deletions(-)

Comments

Jarkko Sakkinen Dec. 29, 2016, 12:58 p.m. UTC | #1
On Thu, Dec 22, 2016 at 12:15:33PM -0800, Sean Christopherson wrote:
> Call sgx_free_encl_page directly from sgx_ewb to make it more clear
> why sgx_free_encl_page is called with(out) SGX_FREE_SKIP_EREMOVE.
> This removes the need to return success/failure from sgx_ewb, and
> eliminates the associated tracking in sgx_write_pages.
> 
> Do not invalidate the enclave if it is already dead.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  drivers/platform/x86/intel_sgx_page_cache.c | 65 +++++++++++++----------------
>  1 file changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> index f4833a0..45964b4 100644
> --- a/drivers/platform/x86/intel_sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> @@ -232,6 +232,15 @@ static void sgx_etrack(struct sgx_epc_page *epc_page)
>  	sgx_put_epc_page(epc);
>  }
>  
> +static void sgx_free_encl_page(struct sgx_encl_page *encl_page,
> +			       struct sgx_encl *encl,
> +			       unsigned int flags)
> +{
> +	sgx_free_page(encl_page->epc_page, encl, flags);
> +	encl_page->epc_page = NULL;
> +	encl_page->flags &= ~SGX_ENCL_PAGE_RESERVED;
> +}
> +
>  static int __sgx_ewb(struct sgx_encl *encl,
>  		     struct sgx_encl_page *encl_page)
>  {
> @@ -267,36 +276,29 @@ static int __sgx_ewb(struct sgx_encl *encl,
>  	return ret;
>  }
>  
> -static bool sgx_ewb(struct sgx_encl *encl,
> -		    struct sgx_encl_page *entry)
> +static void sgx_ewb(struct sgx_encl *encl,
> +		    struct sgx_encl_page *encl_page)
>  {
> -	int ret = __sgx_ewb(encl, entry);
> +	int ret = __sgx_ewb(encl, encl_page);
>  
>  	if (ret == SGX_NOT_TRACKED) {
>  		/* slow path, IPI needed */
>  		smp_call_function(sgx_ipi_cb, NULL, 1);
> -		ret = __sgx_ewb(encl, entry);
> +		ret = __sgx_ewb(encl, encl_page);
>  	}
>  
> -	if (ret) {
> -		/* make enclave inaccessible */
> -		sgx_invalidate(encl);
> -		smp_call_function(sgx_ipi_cb, NULL, 1);
> -		if (ret > 0)
> -			sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
> -		return false;
> +	if (ret == SGX_SUCCESS) {
> +		sgx_free_encl_page(encl_page, encl, SGX_FREE_SKIP_EREMOVE);
                return true;

/Jarkko


> +	} else {
> +		sgx_free_encl_page(encl_page, encl, 0);
> +		if (!(encl->flags & SGX_ENCL_DEAD)) {
> +			/* make enclave inaccessible */
> +			sgx_invalidate(encl);
> +			smp_call_function(sgx_ipi_cb, NULL, 1);
> +			if (ret > 0)
> +				sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
> +		}
>  	}
> -
> -	return true;
> -}
> -
> -void sgx_free_encl_page(struct sgx_encl_page *entry,
> -		    struct sgx_encl *encl,
> -		    unsigned int flags)
> -{
> -	sgx_free_page(entry->epc_page, encl, flags);
> -	entry->epc_page = NULL;
> -	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
>  }
>  
>  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
> @@ -304,7 +306,6 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  	struct sgx_encl_page *entry;
>  	struct sgx_encl_page *tmp;
>  	struct vm_area_struct *evma;
> -	unsigned int free_flags;
>  
>  	if (list_empty(src))
>  		return;
> @@ -335,25 +336,17 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  					 load_list);
>  		list_del(&entry->load_list);
>  
> -		free_flags = 0;
> -
>  		evma = sgx_find_vma(encl, entry->addr);
>  		if (evma) {
> -			if (sgx_ewb(encl, entry))
> -				free_flags = SGX_FREE_SKIP_EREMOVE;
> +			sgx_ewb(encl, entry);
>  			encl->secs_child_cnt--;
> +		} else {
> +			sgx_free_encl_page(entry, encl, 0);
>  		}
> -
> -		sgx_free_encl_page(entry, encl, free_flags);
>  	}
>  
> -	if (!encl->secs_child_cnt && (encl->flags & SGX_ENCL_INITIALIZED)) {
> -		free_flags = 0;
> -		if (sgx_ewb(encl, &encl->secs_page))
> -			free_flags = SGX_FREE_SKIP_EREMOVE;
> -
> -		sgx_free_encl_page(&encl->secs_page, encl, free_flags);
> -	}
> +	if (!encl->secs_child_cnt && (encl->flags & SGX_ENCL_INITIALIZED))
> +		sgx_ewb(encl, &encl->secs_page);
>  
>  	mutex_unlock(&encl->lock);
>  }
> -- 
> 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
Sean Christopherson Jan. 3, 2017, 4:42 p.m. UTC | #2
On Thu, 2016-12-29 at 14:58 +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 22, 2016 at 12:15:33PM -0800, Sean Christopherson wrote:
> > 
> > Call sgx_free_encl_page directly from sgx_ewb to make it more clear
> > why sgx_free_encl_page is called with(out) SGX_FREE_SKIP_EREMOVE.
> > This removes the need to return success/failure from sgx_ewb, and
> > eliminates the associated tracking in sgx_write_pages.
> > 
> > Do not invalidate the enclave if it is already dead.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  drivers/platform/x86/intel_sgx_page_cache.c | 65 +++++++++++++----------------
> >  1 file changed, 29 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> > index f4833a0..45964b4 100644
> > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > @@ -232,6 +232,15 @@ static void sgx_etrack(struct sgx_epc_page *epc_page)
> >  	sgx_put_epc_page(epc);
> >  }
> >  
> > +static void sgx_free_encl_page(struct sgx_encl_page *encl_page,
> > +			       struct sgx_encl *encl,
> > +			       unsigned int flags)
> > +{
> > +	sgx_free_page(encl_page->epc_page, encl, flags);
> > +	encl_page->epc_page = NULL;
> > +	encl_page->flags &= ~SGX_ENCL_PAGE_RESERVED;
> > +}
> > +
> >  static int __sgx_ewb(struct sgx_encl *encl,
> >  		     struct sgx_encl_page *encl_page)
> >  {
> > @@ -267,36 +276,29 @@ static int __sgx_ewb(struct sgx_encl *encl,
> >  	return ret;
> >  }
> >  
> > -static bool sgx_ewb(struct sgx_encl *encl,
> > -		    struct sgx_encl_page *entry)
> > +static void sgx_ewb(struct sgx_encl *encl,
> > +		    struct sgx_encl_page *encl_page)
> >  {
> > -	int ret = __sgx_ewb(encl, entry);
> > +	int ret = __sgx_ewb(encl, encl_page);
> >  
> >  	if (ret == SGX_NOT_TRACKED) {
> >  		/* slow path, IPI needed */
> >  		smp_call_function(sgx_ipi_cb, NULL, 1);
> > -		ret = __sgx_ewb(encl, entry);
> > +		ret = __sgx_ewb(encl, encl_page);
> >  	}
> >  
> > -	if (ret) {
> > -		/* make enclave inaccessible */
> > -		sgx_invalidate(encl);
> > -		smp_call_function(sgx_ipi_cb, NULL, 1);
> > -		if (ret > 0)
> > -			sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
> > -		return false;
> > +	if (ret == SGX_SUCCESS) {
> > +		sgx_free_encl_page(encl_page, encl, SGX_FREE_SKIP_EREMOVE);
>                 return true;
> 
> /Jarkko

sgx_ewb no longer returns a bool.
Jarkko Sakkinen Jan. 3, 2017, 5:32 p.m. UTC | #3
On Tue, Jan 03, 2017 at 08:42:19AM -0800, Sean Christopherson wrote:
> On Thu, 2016-12-29 at 14:58 +0200, Jarkko Sakkinen wrote:
> > On Thu, Dec 22, 2016 at 12:15:33PM -0800, Sean Christopherson wrote:
> > > 
> > > Call sgx_free_encl_page directly from sgx_ewb to make it more clear
> > > why sgx_free_encl_page is called with(out) SGX_FREE_SKIP_EREMOVE.
> > > This removes the need to return success/failure from sgx_ewb, and
> > > eliminates the associated tracking in sgx_write_pages.
> > > 
> > > Do not invalidate the enclave if it is already dead.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > ---
> > >  drivers/platform/x86/intel_sgx_page_cache.c | 65 +++++++++++++----------------
> > >  1 file changed, 29 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> > > index f4833a0..45964b4 100644
> > > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > > @@ -232,6 +232,15 @@ static void sgx_etrack(struct sgx_epc_page *epc_page)
> > >  	sgx_put_epc_page(epc);
> > >  }
> > >  
> > > +static void sgx_free_encl_page(struct sgx_encl_page *encl_page,
> > > +			       struct sgx_encl *encl,
> > > +			       unsigned int flags)
> > > +{
> > > +	sgx_free_page(encl_page->epc_page, encl, flags);
> > > +	encl_page->epc_page = NULL;
> > > +	encl_page->flags &= ~SGX_ENCL_PAGE_RESERVED;
> > > +}
> > > +
> > >  static int __sgx_ewb(struct sgx_encl *encl,
> > >  		     struct sgx_encl_page *encl_page)
> > >  {
> > > @@ -267,36 +276,29 @@ static int __sgx_ewb(struct sgx_encl *encl,
> > >  	return ret;
> > >  }
> > >  
> > > -static bool sgx_ewb(struct sgx_encl *encl,
> > > -		    struct sgx_encl_page *entry)
> > > +static void sgx_ewb(struct sgx_encl *encl,
> > > +		    struct sgx_encl_page *encl_page)
> > >  {
> > > -	int ret = __sgx_ewb(encl, entry);
> > > +	int ret = __sgx_ewb(encl, encl_page);
> > >  
> > >  	if (ret == SGX_NOT_TRACKED) {
> > >  		/* slow path, IPI needed */
> > >  		smp_call_function(sgx_ipi_cb, NULL, 1);
> > > -		ret = __sgx_ewb(encl, entry);
> > > +		ret = __sgx_ewb(encl, encl_page);
> > >  	}
> > >  
> > > -	if (ret) {
> > > -		/* make enclave inaccessible */
> > > -		sgx_invalidate(encl);
> > > -		smp_call_function(sgx_ipi_cb, NULL, 1);
> > > -		if (ret > 0)
> > > -			sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
> > > -		return false;
> > > +	if (ret == SGX_SUCCESS) {
> > > +		sgx_free_encl_page(encl_page, encl, SGX_FREE_SKIP_EREMOVE);
> >                 return true;
> > 
> > /Jarkko
> 
> sgx_ewb no longer returns a bool.

I overlooked. Then just "return;".

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
index f4833a0..45964b4 100644
--- a/drivers/platform/x86/intel_sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx_page_cache.c
@@ -232,6 +232,15 @@  static void sgx_etrack(struct sgx_epc_page *epc_page)
 	sgx_put_epc_page(epc);
 }
 
+static void sgx_free_encl_page(struct sgx_encl_page *encl_page,
+			       struct sgx_encl *encl,
+			       unsigned int flags)
+{
+	sgx_free_page(encl_page->epc_page, encl, flags);
+	encl_page->epc_page = NULL;
+	encl_page->flags &= ~SGX_ENCL_PAGE_RESERVED;
+}
+
 static int __sgx_ewb(struct sgx_encl *encl,
 		     struct sgx_encl_page *encl_page)
 {
@@ -267,36 +276,29 @@  static int __sgx_ewb(struct sgx_encl *encl,
 	return ret;
 }
 
-static bool sgx_ewb(struct sgx_encl *encl,
-		    struct sgx_encl_page *entry)
+static void sgx_ewb(struct sgx_encl *encl,
+		    struct sgx_encl_page *encl_page)
 {
-	int ret = __sgx_ewb(encl, entry);
+	int ret = __sgx_ewb(encl, encl_page);
 
 	if (ret == SGX_NOT_TRACKED) {
 		/* slow path, IPI needed */
 		smp_call_function(sgx_ipi_cb, NULL, 1);
-		ret = __sgx_ewb(encl, entry);
+		ret = __sgx_ewb(encl, encl_page);
 	}
 
-	if (ret) {
-		/* make enclave inaccessible */
-		sgx_invalidate(encl);
-		smp_call_function(sgx_ipi_cb, NULL, 1);
-		if (ret > 0)
-			sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
-		return false;
+	if (ret == SGX_SUCCESS) {
+		sgx_free_encl_page(encl_page, encl, SGX_FREE_SKIP_EREMOVE);
+	} else {
+		sgx_free_encl_page(encl_page, encl, 0);
+		if (!(encl->flags & SGX_ENCL_DEAD)) {
+			/* make enclave inaccessible */
+			sgx_invalidate(encl);
+			smp_call_function(sgx_ipi_cb, NULL, 1);
+			if (ret > 0)
+				sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
+		}
 	}
-
-	return true;
-}
-
-void sgx_free_encl_page(struct sgx_encl_page *entry,
-		    struct sgx_encl *encl,
-		    unsigned int flags)
-{
-	sgx_free_page(entry->epc_page, encl, flags);
-	entry->epc_page = NULL;
-	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
 }
 
 static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
@@ -304,7 +306,6 @@  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 	struct sgx_encl_page *entry;
 	struct sgx_encl_page *tmp;
 	struct vm_area_struct *evma;
-	unsigned int free_flags;
 
 	if (list_empty(src))
 		return;
@@ -335,25 +336,17 @@  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 					 load_list);
 		list_del(&entry->load_list);
 
-		free_flags = 0;
-
 		evma = sgx_find_vma(encl, entry->addr);
 		if (evma) {
-			if (sgx_ewb(encl, entry))
-				free_flags = SGX_FREE_SKIP_EREMOVE;
+			sgx_ewb(encl, entry);
 			encl->secs_child_cnt--;
+		} else {
+			sgx_free_encl_page(entry, encl, 0);
 		}
-
-		sgx_free_encl_page(entry, encl, free_flags);
 	}
 
-	if (!encl->secs_child_cnt && (encl->flags & SGX_ENCL_INITIALIZED)) {
-		free_flags = 0;
-		if (sgx_ewb(encl, &encl->secs_page))
-			free_flags = SGX_FREE_SKIP_EREMOVE;
-
-		sgx_free_encl_page(&encl->secs_page, encl, free_flags);
-	}
+	if (!encl->secs_child_cnt && (encl->flags & SGX_ENCL_INITIALIZED))
+		sgx_ewb(encl, &encl->secs_page);
 
 	mutex_unlock(&encl->lock);
 }