| Message ID | 20170116141918.17382-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 16, 2017 at 04:19:18PM +0200, Jarkko Sakkinen wrote: > In order to have well defined semantics for forking move into using > VM_DONTCOPY for the enclave VMA. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > drivers/platform/x86/intel_sgx_main.c | 7 ++----- > drivers/platform/x86/intel_sgx_vma.c | 27 +++++---------------------- > 2 files changed, 7 insertions(+), 27 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c > index 93dd708..b5e414a 100644 > --- a/drivers/platform/x86/intel_sgx_main.c > +++ b/drivers/platform/x86/intel_sgx_main.c > @@ -103,11 +103,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > static int sgx_mmap(struct file *file, struct vm_area_struct *vma) > { > vma->vm_ops = &sgx_vm_ops; > -#if !defined(VM_RESERVED) > - vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO; > -#else > - vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO; > -#endif > + vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO | > + VM_DONTCOPY; > > return 0; > } > diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c > index e670405..11d1c3b 100644 > --- a/drivers/platform/x86/intel_sgx_vma.c > +++ b/drivers/platform/x86/intel_sgx_vma.c > @@ -71,34 +71,18 @@ > static void sgx_vma_open(struct vm_area_struct *vma) > { > struct sgx_encl *encl = vma->vm_private_data; > - > - /* When forking for the second time vm_private_data is already set to > - * NULL. > - */ > - if (!encl) { > - zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start); > + if (!encl) > return; > - } > - > - /* Invalidate enclave when the process has been forked. */ > - mutex_lock(&encl->lock); > - if (encl->mm != vma->vm_mm) { > - sgx_invalidate(encl); > - vma->vm_private_data = NULL; > - } > - mutex_unlock(&encl->lock); > > - if (vma->vm_private_data) > - kref_get(&encl->refcount); > + /* kref cannot underflow because ECREATE ioctl checks that there is only > + * one single VMA for the enclave before proceeding. > + */ This is not entirely correctly stated. It should say that nullity check prevents the underflow and the check in ECREATE ensures that kref count is always in sync. /Jarkko
On Mon, Jan 16, 2017 at 04:19:18PM +0200, Jarkko Sakkinen wrote: > In order to have well defined semantics for forking move into using > VM_DONTCOPY for the enclave VMA. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Reported-by: Jethro Beekman <jethro@fortanix.com> Jethro, does this address your problem or not? /Jarkko > --- > drivers/platform/x86/intel_sgx_main.c | 7 ++----- > drivers/platform/x86/intel_sgx_vma.c | 27 +++++---------------------- > 2 files changed, 7 insertions(+), 27 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c > index 93dd708..b5e414a 100644 > --- a/drivers/platform/x86/intel_sgx_main.c > +++ b/drivers/platform/x86/intel_sgx_main.c > @@ -103,11 +103,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > static int sgx_mmap(struct file *file, struct vm_area_struct *vma) > { > vma->vm_ops = &sgx_vm_ops; > -#if !defined(VM_RESERVED) > - vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO; > -#else > - vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO; > -#endif > + vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO | > + VM_DONTCOPY; > > return 0; > } > diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c > index e670405..11d1c3b 100644 > --- a/drivers/platform/x86/intel_sgx_vma.c > +++ b/drivers/platform/x86/intel_sgx_vma.c > @@ -71,34 +71,18 @@ > static void sgx_vma_open(struct vm_area_struct *vma) > { > struct sgx_encl *encl = vma->vm_private_data; > - > - /* When forking for the second time vm_private_data is already set to > - * NULL. > - */ > - if (!encl) { > - zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start); > + if (!encl) > return; > - } > - > - /* Invalidate enclave when the process has been forked. */ > - mutex_lock(&encl->lock); > - if (encl->mm != vma->vm_mm) { > - sgx_invalidate(encl); > - vma->vm_private_data = NULL; > - } > - mutex_unlock(&encl->lock); > > - if (vma->vm_private_data) > - kref_get(&encl->refcount); > + /* kref cannot underflow because ECREATE ioctl checks that there is only > + * one single VMA for the enclave before proceeding. > + */ > + kref_get(&encl->refcount); > } > > static void sgx_vma_close(struct vm_area_struct *vma) > { > struct sgx_encl *encl = vma->vm_private_data; > - > - /* When forking for the second time vm_private_data is already set to > - * NULL. > - */ > if (!encl) > return; > > @@ -106,7 +90,6 @@ static void sgx_vma_close(struct vm_area_struct *vma) > zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start); > encl->flags |= SGX_ENCL_DEAD; > mutex_unlock(&encl->lock); > - > kref_put(&encl->refcount, sgx_encl_release); > } > > -- > 2.9.3 >
On 2017-01-17 10:07, Jarkko Sakkinen wrote: > On Mon, Jan 16, 2017 at 04:19:18PM +0200, Jarkko Sakkinen wrote: >> In order to have well defined semantics for forking move into using >> VM_DONTCOPY for the enclave VMA. >> >> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Reported-by: Jethro Beekman <jethro@fortanix.com> > > Jethro, does this address your problem or not? That seems to be the case yes, thanks. Jethro
On Thu, Jan 19, 2017 at 11:01:32PM -0800, Jethro Beekman wrote: > On 2017-01-17 10:07, Jarkko Sakkinen wrote: > > On Mon, Jan 16, 2017 at 04:19:18PM +0200, Jarkko Sakkinen wrote: > > > In order to have well defined semantics for forking move into using > > > VM_DONTCOPY for the enclave VMA. > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Reported-by: Jethro Beekman <jethro@fortanix.com> > > > > Jethro, does this address your problem or not? > > That seems to be the case yes, thanks. > > Jethro OK, I will apply the patch with your Tested/Reviewed-by. /Jarkko
diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c index 93dd708..b5e414a 100644 --- a/drivers/platform/x86/intel_sgx_main.c +++ b/drivers/platform/x86/intel_sgx_main.c @@ -103,11 +103,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) static int sgx_mmap(struct file *file, struct vm_area_struct *vma) { vma->vm_ops = &sgx_vm_ops; -#if !defined(VM_RESERVED) - vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO; -#else - vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO; -#endif + vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO | + VM_DONTCOPY; return 0; } diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c index e670405..11d1c3b 100644 --- a/drivers/platform/x86/intel_sgx_vma.c +++ b/drivers/platform/x86/intel_sgx_vma.c @@ -71,34 +71,18 @@ static void sgx_vma_open(struct vm_area_struct *vma) { struct sgx_encl *encl = vma->vm_private_data; - - /* When forking for the second time vm_private_data is already set to - * NULL. - */ - if (!encl) { - zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start); + if (!encl) return; - } - - /* Invalidate enclave when the process has been forked. */ - mutex_lock(&encl->lock); - if (encl->mm != vma->vm_mm) { - sgx_invalidate(encl); - vma->vm_private_data = NULL; - } - mutex_unlock(&encl->lock); - if (vma->vm_private_data) - kref_get(&encl->refcount); + /* kref cannot underflow because ECREATE ioctl checks that there is only + * one single VMA for the enclave before proceeding. + */ + kref_get(&encl->refcount); } static void sgx_vma_close(struct vm_area_struct *vma) { struct sgx_encl *encl = vma->vm_private_data; - - /* When forking for the second time vm_private_data is already set to - * NULL. - */ if (!encl) return; @@ -106,7 +90,6 @@ static void sgx_vma_close(struct vm_area_struct *vma) zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start); encl->flags |= SGX_ENCL_DEAD; mutex_unlock(&encl->lock); - kref_put(&encl->refcount, sgx_encl_release); }
In order to have well defined semantics for forking move into using VM_DONTCOPY for the enclave VMA. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/platform/x86/intel_sgx_main.c | 7 ++----- drivers/platform/x86/intel_sgx_vma.c | 27 +++++---------------------- 2 files changed, 7 insertions(+), 27 deletions(-)