diff mbox

[intel-sgx-kernel-dev,v4,4/8] intel_sgx: lock the enclave for the entire EPC eviction flow

Message ID 20161201205632.8593-5-jarkko.sakkinen@linux.intel.com
State New, archived
Headers show

Commit Message

Jarkko Sakkinen Dec. 1, 2016, 8:56 p.m. UTC
From: Sean Christopherson <sean.j.christopherson@intel.com>

The SGX architecture does not allow multiple ETRACK sequences to be
in flight at the same time if the enclave being tracked has one or
more active threads in the enclave.  A second ETRACK will fail due
to PREV_TRK_INCMPL if it is attempted before EWB is executed on all
EBLOCKed pages (in the presence of active enclave threads).

Currently the driver releases the enclave's lock after ETRACK and
then reacquires the lock prior to EWB.  Releasing the lock from the
thread that performed ETRACK, T1, allows a different thread, T2, to
acquire the lock for its own ETRACK.  T2's ETRACK will fail due to
the architectural restrictions if a third thread, T3, is executing
inside the enclave whose pages are being swapped.

    T3: <active in enclave E1>
    T1: lock_mutex(E1)
    T1: ETRACK(E1)
    T1: unlock_mutex(E1)
    T2: lock_mutex(E1)
    T2: ETRACK(E1) fails

ETRACK failing is not fatal in and of itself, but the driver is not
designed to handle an ETRACK failure (BUG_ON non-zero return code).
Rather than modifying the driver to handle the failing path, e.g by
kicking T3 out of the enclave with an IPI, simply lock the enclave
for the entire ETRACK->EWB sequence to prevent multiple concurrent
ETRACKs on a single enclave.  Holding the lock for ETRACK->EWB does
not negatively impact performance in any way.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/platform/x86/intel_sgx_page_cache.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

Comments

Jarkko Sakkinen Dec. 2, 2016, 10:27 a.m. UTC | #1
On Thu, Dec 01, 2016 at 10:56:28PM +0200, Jarkko Sakkinen wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> The SGX architecture does not allow multiple ETRACK sequences to be
> in flight at the same time if the enclave being tracked has one or
> more active threads in the enclave.  A second ETRACK will fail due
> to PREV_TRK_INCMPL if it is attempted before EWB is executed on all
> EBLOCKed pages (in the presence of active enclave threads).
> 
> Currently the driver releases the enclave's lock after ETRACK and
> then reacquires the lock prior to EWB.  Releasing the lock from the
> thread that performed ETRACK, T1, allows a different thread, T2, to
> acquire the lock for its own ETRACK.  T2's ETRACK will fail due to
> the architectural restrictions if a third thread, T3, is executing
> inside the enclave whose pages are being swapped.
> 
>     T3: <active in enclave E1>
>     T1: lock_mutex(E1)
>     T1: ETRACK(E1)
>     T1: unlock_mutex(E1)
>     T2: lock_mutex(E1)
>     T2: ETRACK(E1) fails
> 
> ETRACK failing is not fatal in and of itself, but the driver is not
> designed to handle an ETRACK failure (BUG_ON non-zero return code).
> Rather than modifying the driver to handle the failing path, e.g by
> kicking T3 out of the enclave with an IPI, simply lock the enclave
> for the entire ETRACK->EWB sequence to prevent multiple concurrent
> ETRACKs on a single enclave.  Holding the lock for ETRACK->EWB does
> not negatively impact performance in any way.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
diff mbox

Patch

diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
index 8b1cc82..c4865d2 100644
--- a/drivers/platform/x86/intel_sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx_page_cache.c
@@ -261,15 +261,14 @@  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 		return;
 	}
 
-	/* EBLOCK */
+	mutex_lock(&encl->lock);
 
+	/* EBLOCK */
 	list_for_each_entry_safe(entry, tmp, src, load_list) {
-		mutex_lock(&encl->lock);
 		evma = sgx_find_vma(encl, entry->addr);
 		if (!evma) {
 			list_del(&entry->load_list);
 			sgx_free_encl_page(entry, encl, 0);
-			mutex_unlock(&encl->lock);
 			continue;
 		}
 
@@ -278,25 +277,18 @@  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 			list_del(&entry->load_list);
 			list_add_tail(&entry->load_list, &encl->load_list);
 			entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
-			mutex_unlock(&encl->lock);
 			continue;
 		}
 
 		zap_vma_ptes(evma, entry->addr, PAGE_SIZE);
 		sgx_eblock(entry->epc_page);
 		cnt++;
-		mutex_unlock(&encl->lock);
 	}
 
 	/* ETRACK */
-
-	mutex_lock(&encl->lock);
 	sgx_etrack(encl->secs_page.epc_page);
-	mutex_unlock(&encl->lock);
 
 	/* EWB */
-
-	mutex_lock(&encl->lock);
 	i = 0;
 
 	while (!list_empty(src)) {