| Message ID | 20190916101803.30726-17-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes and updates for v23 | expand |
On Mon, Sep 16, 2019 at 01:18:02PM +0300, Jarkko Sakkinen wrote: > Since __sgx_encl_eldu() and __sgx_encl_ewb() pin the backing storage in the > same way, consolidate this to sgx_encl_get_backing() replacing > sgx_encl_get_backing_page(). > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Cc: Shay Katz-zamir <shay.katz-zamir@intel.com> > Cc: Serge Ayoun <serge.ayoun@intel.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/encl.c | 86 +++++++++++++++++++------------ > arch/x86/kernel/cpu/sgx/encl.h | 20 +++---- > arch/x86/kernel/cpu/sgx/reclaim.c | 39 ++++---------- > 3 files changed, 71 insertions(+), 74 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 66762b9c1517..a71414ce05b1 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -17,11 +17,8 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page); > struct sgx_encl *encl = encl_page->encl; > struct sgx_pageinfo pginfo; > - unsigned long pcmd_offset; > - struct page *backing; > + struct sgx_backing b; > pgoff_t page_index; > - pgoff_t pcmd_index; > - struct page *pcmd; > int ret; > > if (secs_page) > @@ -29,24 +26,14 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > else > page_index = PFN_DOWN(encl->size); > > - pcmd_index = sgx_pcmd_index(encl, page_index); > - pcmd_offset = sgx_pcmd_offset(page_index); > - > - backing = sgx_encl_get_backing_page(encl, page_index); > - if (IS_ERR(backing)) { > - ret = PTR_ERR(backing); > - goto err_backing; > - } > - > - pcmd = sgx_encl_get_backing_page(encl, pcmd_index); > - if (IS_ERR(pcmd)) { > - ret = PTR_ERR(pcmd); > - goto err_pcmd; > - } > + ret = sgx_encl_get_backing(encl, page_index, &b); > + if (ret) > + return ret; > > pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page); > - pginfo.contents = (unsigned long)kmap_atomic(backing); > - pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset; > + pginfo.contents = (unsigned long)kmap_atomic(b.contents); > + pginfo.metadata = (unsigned long)kmap_atomic(b.pcmd) + > + b.pcmd_offset; > > if (secs_page) > pginfo.secs = (u64)sgx_epc_addr(secs_page); > @@ -62,15 +49,12 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > ret = -EFAULT; > } > > - kunmap_atomic((void *)(unsigned long)(pginfo.metadata - pcmd_offset)); > + kunmap_atomic((void *)(unsigned long)(pginfo.metadata - b.pcmd_offset)); > kunmap_atomic((void *)(unsigned long)pginfo.contents); > > - put_page(pcmd); > + put_page(b.pcmd); > + put_page(b.contents); If we're going to have sgx_encl_get_backing(), it should be paired with sgx_encl_put_backing(), e.g. it's not immediately obvious whose doing the "get page" corresponding to the put_page() calls.
On Tue, Sep 17, 2019 at 04:05:18PM -0700, Sean Christopherson wrote: > On Mon, Sep 16, 2019 at 01:18:02PM +0300, Jarkko Sakkinen wrote: > > Since __sgx_encl_eldu() and __sgx_encl_ewb() pin the backing storage in the > > same way, consolidate this to sgx_encl_get_backing() replacing > > sgx_encl_get_backing_page(). > > > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > > Cc: Shay Katz-zamir <shay.katz-zamir@intel.com> > > Cc: Serge Ayoun <serge.ayoun@intel.com> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > arch/x86/kernel/cpu/sgx/encl.c | 86 +++++++++++++++++++------------ > > arch/x86/kernel/cpu/sgx/encl.h | 20 +++---- > > arch/x86/kernel/cpu/sgx/reclaim.c | 39 ++++---------- > > 3 files changed, 71 insertions(+), 74 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > index 66762b9c1517..a71414ce05b1 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > @@ -17,11 +17,8 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > > unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page); > > struct sgx_encl *encl = encl_page->encl; > > struct sgx_pageinfo pginfo; > > - unsigned long pcmd_offset; > > - struct page *backing; > > + struct sgx_backing b; > > pgoff_t page_index; > > - pgoff_t pcmd_index; > > - struct page *pcmd; > > int ret; > > > > if (secs_page) > > @@ -29,24 +26,14 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > > else > > page_index = PFN_DOWN(encl->size); > > > > - pcmd_index = sgx_pcmd_index(encl, page_index); > > - pcmd_offset = sgx_pcmd_offset(page_index); > > - > > - backing = sgx_encl_get_backing_page(encl, page_index); > > - if (IS_ERR(backing)) { > > - ret = PTR_ERR(backing); > > - goto err_backing; > > - } > > - > > - pcmd = sgx_encl_get_backing_page(encl, pcmd_index); > > - if (IS_ERR(pcmd)) { > > - ret = PTR_ERR(pcmd); > > - goto err_pcmd; > > - } > > + ret = sgx_encl_get_backing(encl, page_index, &b); > > + if (ret) > > + return ret; > > > > pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page); > > - pginfo.contents = (unsigned long)kmap_atomic(backing); > > - pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset; > > + pginfo.contents = (unsigned long)kmap_atomic(b.contents); > > + pginfo.metadata = (unsigned long)kmap_atomic(b.pcmd) + > > + b.pcmd_offset; > > > > if (secs_page) > > pginfo.secs = (u64)sgx_epc_addr(secs_page); > > @@ -62,15 +49,12 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > > ret = -EFAULT; > > } > > > > - kunmap_atomic((void *)(unsigned long)(pginfo.metadata - pcmd_offset)); > > + kunmap_atomic((void *)(unsigned long)(pginfo.metadata - b.pcmd_offset)); > > kunmap_atomic((void *)(unsigned long)pginfo.contents); > > > > - put_page(pcmd); > > + put_page(b.pcmd); > > + put_page(b.contents); > > If we're going to have sgx_encl_get_backing(), it should be paired with > sgx_encl_put_backing(), e.g. it's not immediately obvious whose doing the > "get page" corresponding to the put_page() calls. OK, I can add that. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 66762b9c1517..a71414ce05b1 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -17,11 +17,8 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page); struct sgx_encl *encl = encl_page->encl; struct sgx_pageinfo pginfo; - unsigned long pcmd_offset; - struct page *backing; + struct sgx_backing b; pgoff_t page_index; - pgoff_t pcmd_index; - struct page *pcmd; int ret; if (secs_page) @@ -29,24 +26,14 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, else page_index = PFN_DOWN(encl->size); - pcmd_index = sgx_pcmd_index(encl, page_index); - pcmd_offset = sgx_pcmd_offset(page_index); - - backing = sgx_encl_get_backing_page(encl, page_index); - if (IS_ERR(backing)) { - ret = PTR_ERR(backing); - goto err_backing; - } - - pcmd = sgx_encl_get_backing_page(encl, pcmd_index); - if (IS_ERR(pcmd)) { - ret = PTR_ERR(pcmd); - goto err_pcmd; - } + ret = sgx_encl_get_backing(encl, page_index, &b); + if (ret) + return ret; pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page); - pginfo.contents = (unsigned long)kmap_atomic(backing); - pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset; + pginfo.contents = (unsigned long)kmap_atomic(b.contents); + pginfo.metadata = (unsigned long)kmap_atomic(b.pcmd) + + b.pcmd_offset; if (secs_page) pginfo.secs = (u64)sgx_epc_addr(secs_page); @@ -62,15 +49,12 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, ret = -EFAULT; } - kunmap_atomic((void *)(unsigned long)(pginfo.metadata - pcmd_offset)); + kunmap_atomic((void *)(unsigned long)(pginfo.metadata - b.pcmd_offset)); kunmap_atomic((void *)(unsigned long)pginfo.contents); - put_page(pcmd); + put_page(b.pcmd); + put_page(b.contents); -err_pcmd: - put_page(backing); - -err_backing: return ret; } @@ -537,14 +521,8 @@ void sgx_encl_release(struct kref *ref) kfree(encl); } -/** - * sgx_encl_encl_get_backing_page() - Pin the backing page - * @encl: an enclave - * @index: page index - * - * Return: the pinned backing page - */ -struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, pgoff_t index) +static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, + pgoff_t index) { struct inode *inode = encl->backing->f_path.dentry->d_inode; struct address_space *mapping = inode->i_mapping; @@ -553,6 +531,46 @@ struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, pgoff_t index) return shmem_read_mapping_page_gfp(mapping, index, gfpmask); } +/** + * sgx_encl_get_backing() - Access the backing storage + * @encl: an enclave + * @page_index: enclave page index + * @backing: data for accessing backing storage for the page + * + * Pin the backing storage pages for storing the encrypted contents and Paging + * Crypto MetaData (PCMD) of an enclave page. + * + * Return: + * 0 on success, + * -errno otherwise. + */ +int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, + struct sgx_backing *backing) +{ + pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >> 5); + struct page *contents; + struct page *pcmd; + + contents = sgx_encl_get_backing_page(encl, page_index); + if (IS_ERR(contents)) + return PTR_ERR(contents); + + pcmd = sgx_encl_get_backing_page(encl, pcmd_index); + if (IS_ERR(pcmd)) { + put_page(contents); + return PTR_ERR(pcmd); + } + + backing->page_index = page_index; + backing->contents = contents; + backing->pcmd = pcmd; + backing->pcmd_offset = + (page_index & (PAGE_SIZE / sizeof(struct sgx_pcmd) - 1)) * + sizeof(struct sgx_pcmd); + + return 0; +} + static int sgx_encl_test_and_clear_young_cb(pte_t *ptep, unsigned long addr, void *data) { diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h index c7abca1fcb9d..59bc4a5bf7e6 100644 --- a/arch/x86/kernel/cpu/sgx/encl.h +++ b/arch/x86/kernel/cpu/sgx/encl.h @@ -97,23 +97,19 @@ struct sgx_va_page { extern const struct vm_operations_struct sgx_vm_ops; -static inline pgoff_t sgx_pcmd_index(struct sgx_encl *encl, - pgoff_t page_index) -{ - return PFN_DOWN(encl->size) + 1 + (page_index >> 5); -} - -static inline unsigned long sgx_pcmd_offset(pgoff_t page_index) -{ - return (page_index & (PAGE_SIZE / sizeof(struct sgx_pcmd) - 1)) * - sizeof(struct sgx_pcmd); -} +struct sgx_backing { + pgoff_t page_index; + struct page *contents; + struct page *pcmd; + unsigned long pcmd_offset; +}; int sgx_encl_find(struct mm_struct *mm, unsigned long addr, struct vm_area_struct **vma); void sgx_encl_destroy(struct sgx_encl *encl); void sgx_encl_release(struct kref *ref); -struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, pgoff_t index); +int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, + struct sgx_backing *backing); 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); diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c index 2e04a923d8dc..7d628a1388e2 100644 --- a/arch/x86/kernel/cpu/sgx/reclaim.c +++ b/arch/x86/kernel/cpu/sgx/reclaim.c @@ -209,47 +209,30 @@ static int __sgx_encl_ewb(struct sgx_encl *encl, struct sgx_epc_page *epc_page, unsigned int page_index) { struct sgx_pageinfo pginfo; - unsigned long pcmd_offset; - struct page *backing; - pgoff_t pcmd_index; - struct page *pcmd; + struct sgx_backing b; int ret; - pcmd_index = sgx_pcmd_index(encl, page_index); - pcmd_offset = sgx_pcmd_offset(page_index); - - backing = sgx_encl_get_backing_page(encl, page_index); - if (IS_ERR(backing)) { - ret = PTR_ERR(backing); - goto err_backing; - } - - pcmd = sgx_encl_get_backing_page(encl, pcmd_index); - if (IS_ERR(pcmd)) { - ret = PTR_ERR(pcmd); - goto err_pcmd; - } + ret = sgx_encl_get_backing(encl, page_index, &b); + if (ret) + return ret; pginfo.addr = 0; - pginfo.contents = (unsigned long)kmap_atomic(backing); - pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset; + pginfo.contents = (unsigned long)kmap_atomic(b.contents); + pginfo.metadata = (unsigned long)kmap_atomic(b.pcmd) + b.pcmd_offset; pginfo.secs = 0; ret = __ewb(&pginfo, sgx_epc_addr(epc_page), sgx_epc_addr(va_page->epc_page) + va_offset); - kunmap_atomic((void *)(unsigned long)(pginfo.metadata - pcmd_offset)); + kunmap_atomic((void *)(unsigned long)(pginfo.metadata - b.pcmd_offset)); kunmap_atomic((void *)(unsigned long)pginfo.contents); if (!ret) { - set_page_dirty(pcmd); - set_page_dirty(backing); + set_page_dirty(b.pcmd); + set_page_dirty(b.contents); } - put_page(pcmd); - -err_pcmd: - put_page(backing); + put_page(b.pcmd); + put_page(b.contents); -err_backing: return ret; }
Since __sgx_encl_eldu() and __sgx_encl_ewb() pin the backing storage in the same way, consolidate this to sgx_encl_get_backing() replacing sgx_encl_get_backing_page(). Cc: Sean Christopherson <sean.j.christopherson@intel.com> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com> Cc: Serge Ayoun <serge.ayoun@intel.com> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 86 +++++++++++++++++++------------ arch/x86/kernel/cpu/sgx/encl.h | 20 +++---- arch/x86/kernel/cpu/sgx/reclaim.c | 39 ++++---------- 3 files changed, 71 insertions(+), 74 deletions(-)