| Message ID | 20161208123828.21834-9-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-12-08 at 14:38 +0200, Jarkko Sakkinen wrote: > Page table should not be manipulated after all user processes cease > to exist. This can result page table inconsistency errors: > > BUG: non-zero nr_ptes on freeing mm: 1 > > This commit fixes the issue by invalidating the enclave after all > user processes have been died. > > Reported-by: Sean Christopherson <sean.j.christopherson@intel.com> > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c > index f2a2ed1..49dd664 100644 > --- a/drivers/platform/x86/intel_sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > @@ -227,12 +227,13 @@ static bool sgx_ewb(struct sgx_encl *encl, > { > int ret = __sgx_ewb(encl, entry, backing); > > - /* Only kick out threads with an IPI if needed. */ > + /* fast path */ > if (ret == SGX_NOT_TRACKED) { > smp_call_function(sgx_ipi_cb, NULL, 1); > ret = __sgx_ewb(encl, entry, backing); > } > > + /* slow path */ > if (ret) { > /* Make enclave inaccessible. */ > sgx_invalidate(encl); Nitpick, but can we remove the "fast path" and "slow path" comments? The are potentially misleading as they seem to imply that the "ret == SGX_NOT_TRACKED" case is the fast path and the fall-through is the slow path, whereas in my view the first __sgx_ewb is the fast path, the IPI case is the slow path and sgx_invalidate is effectively an abort path.
On Tue, Dec 13, 2016 at 11:01:56AM -0800, Sean Christopherson wrote: > On Thu, 2016-12-08 at 14:38 +0200, Jarkko Sakkinen wrote: > > Page table should not be manipulated after all user processes cease > > to exist. This can result page table inconsistency errors: > > > > BUG: non-zero nr_ptes on freeing mm: 1 > > > > This commit fixes the issue by invalidating the enclave after all > > user processes have been died. > > > > Reported-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c > > index f2a2ed1..49dd664 100644 > > --- a/drivers/platform/x86/intel_sgx_page_cache.c > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > > @@ -227,12 +227,13 @@ static bool sgx_ewb(struct sgx_encl *encl, > > { > > int ret = __sgx_ewb(encl, entry, backing); > > > > - /* Only kick out threads with an IPI if needed. */ > > + /* fast path */ > > if (ret == SGX_NOT_TRACKED) { > > smp_call_function(sgx_ipi_cb, NULL, 1); > > ret = __sgx_ewb(encl, entry, backing); > > } > > > > + /* slow path */ > > if (ret) { > > /* Make enclave inaccessible. */ > > sgx_invalidate(encl); > > Nitpick, but can we remove the "fast path" and "slow path" comments? > The are potentially misleading as they seem to imply that the "ret == > SGX_NOT_TRACKED" case is the fast path and the fall-through is the > slow path, whereas in my view the first __sgx_ewb is the fast path, > the IPI case is the slow path and sgx_invalidate is effectively an > abort path. You are absolutely right. They were incorrectly placed by me. Thanks for noticing this. Given that the function does not have much complexity at all they could be simply dropped. /Jarkko
On Tue, Dec 13, 2016 at 09:14:02PM +0200, Jarkko Sakkinen wrote: > On Tue, Dec 13, 2016 at 11:01:56AM -0800, Sean Christopherson wrote: > > On Thu, 2016-12-08 at 14:38 +0200, Jarkko Sakkinen wrote: > > > Page table should not be manipulated after all user processes cease > > > to exist. This can result page table inconsistency errors: > > > > > > BUG: non-zero nr_ptes on freeing mm: 1 > > > > > > This commit fixes the issue by invalidating the enclave after all > > > user processes have been died. > > > > > > Reported-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > --- > > > > > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c > > > index f2a2ed1..49dd664 100644 > > > --- a/drivers/platform/x86/intel_sgx_page_cache.c > > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > > > @@ -227,12 +227,13 @@ static bool sgx_ewb(struct sgx_encl *encl, > > > { > > > int ret = __sgx_ewb(encl, entry, backing); > > > > > > - /* Only kick out threads with an IPI if needed. */ > > > + /* fast path */ > > > if (ret == SGX_NOT_TRACKED) { > > > smp_call_function(sgx_ipi_cb, NULL, 1); > > > ret = __sgx_ewb(encl, entry, backing); > > > } > > > > > > + /* slow path */ > > > if (ret) { > > > /* Make enclave inaccessible. */ > > > sgx_invalidate(encl); > > > > Nitpick, but can we remove the "fast path" and "slow path" comments? > > The are potentially misleading as they seem to imply that the "ret == > > SGX_NOT_TRACKED" case is the fast path and the fall-through is the > > slow path, whereas in my view the first __sgx_ewb is the fast path, > > the IPI case is the slow path and sgx_invalidate is effectively an > > abort path. > > You are absolutely right. They were incorrectly placed by me. > Thanks for noticing this. Given that the function does not have > much complexity at all they could be simply dropped. I do not consider this even a nitpick because they are in wrong places. It's a valid point. /Jarkko
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index b8db914..f53a759 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1031,6 +1031,7 @@ config INTEL_SGX tristate "Intel(R) SGX Driver" default n depends on X86 + 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. The code diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h index 018cd03..f1ff194 100644 --- a/drivers/platform/x86/intel_sgx.h +++ b/drivers/platform/x86/intel_sgx.h @@ -67,6 +67,7 @@ #include <linux/rwsem.h> #include <linux/sched.h> #include <linux/workqueue.h> +#include <linux/mmu_notifier.h> #define SGX_EINIT_SPIN_COUNT 20 #define SGX_EINIT_SLEEP_COUNT 50 @@ -151,6 +152,7 @@ struct sgx_encl { struct sgx_encl_page secs_page; struct sgx_tgid_ctx *tgid_ctx; struct list_head encl_list; + struct mmu_notifier mmu_notifier; }; extern struct workqueue_struct *sgx_add_page_wq; diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c index d0113b0..aaccbc8 100644 --- a/drivers/platform/x86/intel_sgx_ioctl.c +++ b/drivers/platform/x86/intel_sgx_ioctl.c @@ -471,6 +471,21 @@ static int sgx_init_page(struct sgx_encl *encl, return 0; } +static void sgx_mmu_notifier_release(struct mmu_notifier *mn, + struct mm_struct *mm) +{ + struct sgx_encl *encl = + container_of(mn, struct sgx_encl, mmu_notifier); + + mutex_lock(&encl->lock); + encl->flags |= SGX_ENCL_DEAD; + mutex_unlock(&encl->lock); +} + +static const struct mmu_notifier_ops sgx_mmu_notifier_ops = { + .release = sgx_mmu_notifier_release, +}; + /** * sgx_ioc_enclave_create - handler for SGX_IOC_ENCLAVE_CREATE * @@ -572,6 +587,14 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, if (secs->flags & SGX_SECS_A_DEBUG) encl->flags |= SGX_ENCL_DEBUG; + + encl->mmu_notifier.ops = &sgx_mmu_notifier_ops; + ret = mmu_notifier_register(&encl->mmu_notifier, encl->mm); + if (ret) { + encl->mmu_notifier.ops = NULL; + goto out; + } + down_read(¤t->mm->mmap_sem); vma = find_vma(current->mm, secs->base); if (!vma || vma->vm_ops != &sgx_vm_ops || diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c index f2a2ed1..49dd664 100644 --- a/drivers/platform/x86/intel_sgx_page_cache.c +++ b/drivers/platform/x86/intel_sgx_page_cache.c @@ -227,12 +227,13 @@ static bool sgx_ewb(struct sgx_encl *encl, { int ret = __sgx_ewb(encl, entry, backing); - /* Only kick out threads with an IPI if needed. */ + /* fast path */ if (ret == SGX_NOT_TRACKED) { smp_call_function(sgx_ipi_cb, NULL, 1); ret = __sgx_ewb(encl, entry, backing); } + /* slow path */ if (ret) { /* Make enclave inaccessible. */ sgx_invalidate(encl); diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c index d5238c2..f6f7dde0 100644 --- a/drivers/platform/x86/intel_sgx_util.c +++ b/drivers/platform/x86/intel_sgx_util.c @@ -237,6 +237,10 @@ void sgx_encl_release(struct kref *ref) mutex_unlock(&sgx_tgid_ctx_mutex); + if (encl->mmu_notifier.ops) + mmu_notifier_unregister_no_release(&encl->mmu_notifier, + encl->mm); + rb1 = rb_first(&encl->encl_rb); while (rb1) { entry = container_of(rb1, struct sgx_encl_page, node);
Page table should not be manipulated after all user processes cease to exist. This can result page table inconsistency errors: BUG: non-zero nr_ptes on freeing mm: 1 This commit fixes the issue by invalidating the enclave after all user processes have been died. Reported-by: Sean Christopherson <sean.j.christopherson@intel.com> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/platform/x86/Kconfig | 1 + drivers/platform/x86/intel_sgx.h | 2 ++ drivers/platform/x86/intel_sgx_ioctl.c | 23 +++++++++++++++++++++++ drivers/platform/x86/intel_sgx_page_cache.c | 3 ++- drivers/platform/x86/intel_sgx_util.c | 4 ++++ 5 files changed, 32 insertions(+), 1 deletion(-)