x86/sgx: Use SRCU and mmu_notifier

Move to SRCU to eliminate custom list walker, and optionally use mmu_notifier in lieu of refcounting VMAs to avoid defining ->close.

v2: Rebase to latest master.

Sean Christopherson (2):
x86/sgx: Use SRCU to protect mm_list during reclaim x86/sgx: Use mmu_notifier.release() instead of per-vma refcounting

 arch/x86/Kconfig                      |   2 +
 arch/x86/kernel/cpu/sgx/driver/main.c |  60 +++++----
 arch/x86/kernel/cpu/sgx/encl.c        | 182 +++++++++++++-------------
 arch/x86/kernel/cpu/sgx/encl.h        |  12 +-
 arch/x86/kernel/cpu/sgx/reclaim.c     |  71 ++++------
 5 files changed, 158 insertions(+), 169 deletions(-)

2.22.0

Reclaiming enclaves faces a bit of a conundrum when it comes to lock ordering. The reclaim flows need to take mmap_sem for read, e.g. to age and zap PTEs on arbitrary mm_structs. But reclaim must first walk the enclave's list of mm_structs, which could be modified asynchronously to reclaim. Because modifying the list of mm_structs is done in reaction to vma changes, i.e. with mmap_sem held exclusively, taking enclave's mm_lock to protect the list walk in reclaim would lead to deadlocks due to conflicting lock ordering. To avoid this, SGX currently uses a custom walker that drops mm_lock and restarts the walk as needed.

Use SRCU to protect reclaim instead of using a custom walker to avoid the aforementioned lock issues. Using SRCU improves readability in the reclaimer by eliminating the need to juggle mm_lock during reclaim since it can take mmap_sem() underneath srcu_read_lock(). And since relcaim doesn't drop its SRCU read lock, there is no need to grab a reference to encl_mm.

Not taking a reference to encl_mm is not just an optimization, it's also functionally necessary and a major motivation to moving to SRCu. Putting the reference can invoke sgx_encl_mm_release(), which calls synchronize_srcu() and will deadlock if done while holding the SRCU read lock. Not taking a reference paves the way for additional refcounting improvements that would be extremely difficult to implement when using the custom walker due to cyclical dependencies on the refcount.

Speaking of sgx_encl_mm_release(), the whole purpose of using SRCU is that sgx_encl_mm_release() is blocked (if called on another cpu) by synchronize_srcu(), which in turn prevents mmdrop() from freeing the mm_struct while reclaim is in the SRCU critical section. Ultimately, reclaim just needs to ensure mm_struct isn't freed so that it can call mmget_not_zero() to prevent the page tables from being dropped while it accesses PTEs, i.e. it doesn't matter if the encl_mm is dying, reclaim just needs to make sure it's not fully dead.

To avoid calling synchronize_rcu() while holding rcu_read_lock(), use mmput_async() in the reclaimer, e.g. __mmput() closes all VMAs, thus triggering sgx_encl_mm_release() and synchronize_srcu(). Alternatively sgx_encl_mm_release() could always call synchronize_rcu() in a worker thread (see below), but doing __mmput() in a worker thread is desirable from an SGX performance perspective, i.e. doesn't stall the reclaimer CPU to release the mm.

And finally, the last deadlock scenario is if sgx_encl_mm_release() is invoked on an in-use mm_struct, e.g. via munmap().

CPU0 CPU1
munmap()
down_write(&mmap_sem)
srcu_read_lock()

synchronize_srcu()
down_read(&mmap_sem) <- deadlock

Avoid deadlock in this scenario by synchronizing SRCU via a worker thread. SRCU ensures only the liveliness of the mm_struct itself, which is guaranteed by an mmgrab() prior to scheduling the work. The reclaimer is responsible for checking mm_users and the VMAs to ensure it doesn't touch stale PTEs, i.e. delaying synchronization does not affect the reclaimer's responsiblities. The delay does add one new wrinkle in that sgx_encl_mm_add() and sgx_vma_open() can see a dying encl_mm. Previously this was prevented by virtue of sgx_vma_close() being mutually exclusive (the caller must hold down_write(&mmap_sem)). Handle such a case by using kref_get_unless_zero().

---
 arch/x86/Kconfig                      |   1 +
 arch/x86/kernel/cpu/sgx/driver/main.c |  34 ++----
 arch/x86/kernel/cpu/sgx/encl.c        | 165 ++++++++++++++------------
 arch/x86/kernel/cpu/sgx/encl.h        |   9 +-
 arch/x86/kernel/cpu/sgx/reclaim.c     |  71 ++++-------
 5 files changed, 124 insertions(+), 156 deletions(-)

index a0fd17c32521..17558cf48a8a 100644
@@ -1918,6 +1918,7 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
 config INTEL_SGX
 	bool "Intel SGX core functionality"
 	depends on X86_64 && CPU_SUP_INTEL
+	select SRCU
 	---help---
 	  Intel(R) SGX is a set of CPU instructions that can be used by
 	  applications to set aside private regions of code and data, referred

index 2d1943c13879..c4f263adf08f 100644
@@ -25,6 +25,7 @@ u32 sgx_xsave_size_tbl[64];
 static int sgx_open(struct inode *inode, struct file *file)
 {
 	struct sgx_encl *encl;
+	int ret;
 
 	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
 	if (!encl)

@@ -38,6 +39,12 @@ static int sgx_open(struct inode *inode, struct file *file)
 	INIT_LIST_HEAD(&encl->mm_list);
 	spin_lock_init(&encl->mm_lock);
 
+	ret = init_srcu_struct(&encl->srcu);
+	if (ret) {
+		kfree(encl);
+		return ret;
+	}
+
 	file->private_data = encl;
 
 	return 0;

@@ -65,25 +72,6 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
 }
 #endif
 
-static int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
-{
-	struct sgx_encl_mm *encl_mm;
-
-	encl_mm = kzalloc(sizeof(*encl_mm), GFP_KERNEL);
-	if (!encl_mm)
-		return -ENOMEM;
-
-	encl_mm->encl = encl;
-	encl_mm->mm = mm;
-	kref_init(&encl_mm->refcount);
-
-	spin_lock(&encl->mm_lock);
-	list_add(&encl_mm->list, &encl->mm_list);
-	spin_unlock(&encl->mm_lock);
-
-	return 0;
-}
-
 static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = file->private_data;

@@ -94,11 +82,9 @@ static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 	if (ret)
 		return ret;
 
-	if (!sgx_encl_get_mm(encl, vma->vm_mm)) {
-		ret = sgx_encl_mm_add(encl, vma->vm_mm);
-		if (ret)
-			return ret;
-	}
+	ret = sgx_encl_mm_add(encl, vma->vm_mm);
+	if (ret)
+		return ret;
 
 	vma->vm_ops = &sgx_vm_ops;
 	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;

index 19808e7750e4..f1d01a83d9bc 100644
@@ -12,6 +12,7 @@
 #include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/radix-tree.h>
+#include <linux/srcu.h>
 #include <linux/workqueue.h>
 
 /**

@@ -59,6 +60,7 @@ struct sgx_encl_mm {
 	struct mm_struct *mm;
 	struct kref refcount;
 	struct list_head list;
+	struct work_struct release_work;
 };
 
 struct sgx_encl {

@@ -72,6 +74,7 @@ struct sgx_encl {
 	spinlock_t mm_lock;
 	struct file *backing;
 	struct kref refcount;
+	struct srcu_struct srcu;
 	unsigned long base;
 	unsigned long size;
 	unsigned long ssaframesize;

@@ -117,11 +120,7 @@ void sgx_encl_destroy(struct sgx_encl *encl);
 void sgx_encl_release(struct kref *ref);
 pgoff_t sgx_encl_get_index(struct sgx_encl *encl, struct sgx_encl_page *page);
 struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, pgoff_t index);
-struct sgx_encl_mm *sgx_encl_next_mm(struct sgx_encl *encl,
-				     struct sgx_encl_mm *encl_mm, int *iter);
-void sgx_encl_mm_release(struct kref *ref);
-struct sgx_encl_mm *sgx_encl_get_mm(struct sgx_encl *encl,
-				    struct mm_struct *mm);
+int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
 int sgx_encl_test_and_clear_young(struct mm_struct *mm,
 				  struct sgx_encl_page *page);
 struct sgx_encl_page *sgx_encl_reserve_page(struct sgx_encl *encl,

2.22.0

Using per-vma refcounting to track mm_structs associated with an enclave requires hooking .vm_close(), which in turn prevents the mm from merging vmas (precisely to allow refcounting).

Avoid refcounting encl_mm altogether by registering an mmu_notifier at .mmap(), removing the dying encl_mm at mmu_notifier.release() and protecting mm_list during reclaim via a per-enclave SRCU.

Removing refcounting/vm_close() allows merging of enclave vmas, at the cost of delaying removal of encl_mm structs from mm_list, i.e. an mm is disassociated from an enclave when the mm exits or the enclave dies, as opposed to when the last vma (in a given mm) is closed.

The impact of delying encl_mm removal is its memory footprint and whatever overhead is incurred during EPC reclaim (to walk an mm's vmas). Practically speaking, a stale encl_mm will exist for a meaningful amount of time if and only if the enclave is mapped in a long-lived process and then passed off to another long-lived process. It is expected that the vast majority of use cases will not encounter this condition, e.g. even using a daemon to build enclaves should not result in a stale encl_mm as the builder should never need to mmap() the enclave.

Even if there are scenarios that lead to defunct encl_mms, the cost is likely far outweighed by the benefits of reducing the number of vmas across all enclaves.

---
 arch/x86/Kconfig                      |  1 +
 arch/x86/kernel/cpu/sgx/driver/main.c | 26 ++++++++
 arch/x86/kernel/cpu/sgx/encl.c        | 89 ++++++++++++---------------
 arch/x86/kernel/cpu/sgx/encl.h        |  5 +-
 4 files changed, 71 insertions(+), 50 deletions(-)

index 17558cf48a8a..2957dc59e622 100644
@@ -1919,6 +1919,7 @@ config INTEL_SGX
 	bool "Intel SGX core functionality"
 	depends on X86_64 && CPU_SUP_INTEL
 	select SRCU
+	select MMU_NOTIFIER
 	---help---
 	  Intel(R) SGX is a set of CPU instructions that can be used by
 	  applications to set aside private regions of code and data, referred

index c4f263adf08f..3ee6fd716ce4 100644
@@ -53,6 +53,32 @@ static int sgx_open(struct inode *inode, struct file *file)
 static int sgx_release(struct inode *inode, struct file *file)
 {
 	struct sgx_encl *encl = file->private_data;
+	struct sgx_encl_mm *encl_mm;
+
+	/*
+	 * Objects can't be *moved* off an RCU protected list (deletion is ok),
+	 * nor can the object be freed until after synchronize_srcu().
+	 */
+restart:
+	spin_lock(&encl->mm_lock);
+	if (list_empty(&encl->mm_list)) {
+		encl_mm = NULL;
+	} else {
+		encl_mm = list_first_entry(&encl->mm_list, struct sgx_encl_mm,
+					   list);
+		list_del_rcu(&encl_mm->list);
+	}
+	spin_unlock(&encl->mm_lock);
+
+	if (encl_mm) {
+		synchronize_srcu(&encl->srcu);
+
+		mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
+
+		kfree(encl_mm);
+
+		goto restart;
+	}
 
 	mutex_lock(&encl->lock);
 	encl->flags |= SGX_ENCL_DEAD;

index f1d01a83d9bc..8d101b85b06a 100644
@@ -9,6 +9,7 @@
 #include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/mm_types.h>
+#include <linux/mmu_notifier.h>
 #include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/radix-tree.h>

@@ -58,9 +59,9 @@ enum sgx_encl_flags {
 struct sgx_encl_mm {
 	struct sgx_encl *encl;
 	struct mm_struct *mm;
-	struct kref refcount;
 	struct list_head list;
-	struct work_struct release_work;
+	struct mmu_notifier mmu_notifier;
+	struct rcu_head rcu;
 };
 
 struct sgx_encl {

2.22.0

Oh you were fast, thanks :-)

/Jarkko