| Message ID | 1482437735-4722-3-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 22, 2016 at 12:15:31PM -0800, Sean Christopherson wrote: > The unlocked modification of encl->va_pages can cause segfaults if > two or more threads trigger a VA page allocation at the same time. Rather take the lock one step earlier in __encl_add_page. /Jarkko > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > drivers/platform/x86/intel_sgx_ioctl.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c > index 3a4a8fa..b78c552 100644 > --- a/drivers/platform/x86/intel_sgx_ioctl.c > +++ b/drivers/platform/x86/intel_sgx_ioctl.c > @@ -435,7 +435,10 @@ static int sgx_init_page(struct sgx_encl *encl, > > va_page->epc_page = epc_page; > va_offset = sgx_alloc_va_slot(va_page); > + > + mutex_lock(&encl->lock); > list_add(&va_page->list, &encl->va_pages); > + mutex_unlock(&encl->lock); > } > > entry->va_page = va_page; > -- > 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
On Thu, 2016-12-29 at 14:55 +0200, Jarkko Sakkinen wrote: > On Thu, Dec 22, 2016 at 12:15:31PM -0800, Sean Christopherson wrote: > > > > The unlocked modification of encl->va_pages can cause segfaults if > > two or more threads trigger a VA page allocation at the same time. > Rather take the lock one step earlier in __encl_add_page. > > /Jarkko > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > drivers/platform/x86/intel_sgx_ioctl.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c > > index 3a4a8fa..b78c552 100644 > > --- a/drivers/platform/x86/intel_sgx_ioctl.c > > +++ b/drivers/platform/x86/intel_sgx_ioctl.c > > @@ -435,7 +435,10 @@ static int sgx_init_page(struct sgx_encl *encl, > > > > va_page->epc_page = epc_page; > > va_offset = sgx_alloc_va_slot(va_page); > > + > > + mutex_lock(&encl->lock); > > list_add(&va_page->list, &encl->va_pages); > > + mutex_unlock(&encl->lock); > > } > > > > entry->va_page = va_page; Allocating a VA page involves calling sgx_alloc_page, which cannot be done while holding encl->lock. A later patch in the series, which allocates VA slots at eviction time and reserves VA pages at creation time, makes the on-demand locking a little less awkward as the sgx_init_page function becomes sgx_alloc_va_page, i.e. it's more obvious why encl- >lock cannot be held.
On Tue, Jan 03, 2017 at 08:35:54AM -0800, Sean Christopherson wrote: > On Thu, 2016-12-29 at 14:55 +0200, Jarkko Sakkinen wrote: > > On Thu, Dec 22, 2016 at 12:15:31PM -0800, Sean Christopherson wrote: > > > > > > The unlocked modification of encl->va_pages can cause segfaults if > > > two or more threads trigger a VA page allocation at the same time. > > Rather take the lock one step earlier in __encl_add_page. > > > > /Jarkko > > > > > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > --- > > > drivers/platform/x86/intel_sgx_ioctl.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c > > > index 3a4a8fa..b78c552 100644 > > > --- a/drivers/platform/x86/intel_sgx_ioctl.c > > > +++ b/drivers/platform/x86/intel_sgx_ioctl.c > > > @@ -435,7 +435,10 @@ static int sgx_init_page(struct sgx_encl *encl, > > > > > > va_page->epc_page = epc_page; > > > va_offset = sgx_alloc_va_slot(va_page); > > > + > > > + mutex_lock(&encl->lock); > > > list_add(&va_page->list, &encl->va_pages); > > > + mutex_unlock(&encl->lock); > > > } > > > > > > entry->va_page = va_page; > > Allocating a VA page involves calling sgx_alloc_page, which cannot be done while holding encl->lock. A later patch in > the series, which allocates VA slots at eviction time and reserves VA pages at creation time, makes the on-demand > locking a little less awkward as the sgx_init_page function becomes sgx_alloc_va_page, i.e. it's more obvious why encl- > >lock cannot be held. I overlooked when i reviewed this so this is fine. I'm not sure what you mean it being more obvious in a later patch? Why it is less obvious in this? /Jarkko
diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c index 3a4a8fa..b78c552 100644 --- a/drivers/platform/x86/intel_sgx_ioctl.c +++ b/drivers/platform/x86/intel_sgx_ioctl.c @@ -435,7 +435,10 @@ static int sgx_init_page(struct sgx_encl *encl, va_page->epc_page = epc_page; va_offset = sgx_alloc_va_slot(va_page); + + mutex_lock(&encl->lock); list_add(&va_page->list, &encl->va_pages); + mutex_unlock(&encl->lock); } entry->va_page = va_page;
The unlocked modification of encl->va_pages can cause segfaults if two or more threads trigger a VA page allocation at the same time. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- drivers/platform/x86/intel_sgx_ioctl.c | 3 +++ 1 file changed, 3 insertions(+)