| Message ID | 20220301125836.3430-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] x86/sgx: Free backing memory after faulting the enclave page | expand |
On 3/1/22 04:58, Jarkko Sakkinen wrote: > @@ -32,14 +58,16 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > else > page_index = PFN_DOWN(encl->size); > > + page_pcmd_off = sgx_encl_get_backing_page_pcmd_offset(encl, page_index); > + > ret = sgx_encl_lookup_backing(encl, page_index, &b); > if (ret) > return ret; What tree is this against? It looks like it might be on top of Kristen's overcommit series. It would be best if you could test this on top of tip/sgx. Kristen changed code in this area as well.
On Tue, Mar 01, 2022 at 09:54:14AM -0800, Dave Hansen wrote: > On 3/1/22 04:58, Jarkko Sakkinen wrote: > > @@ -32,14 +58,16 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > > else > > page_index = PFN_DOWN(encl->size); > > > > + page_pcmd_off = sgx_encl_get_backing_page_pcmd_offset(encl, page_index); > > + > > ret = sgx_encl_lookup_backing(encl, page_index, &b); > > if (ret) > > return ret; > > What tree is this against? It looks like it might be on top of > Kristen's overcommit series. > > It would be best if you could test this on top of tip/sgx. Kristen > changed code in this area as well. I rebased this against latest stuff and now I did a sanity check: $ git fetch tip remote: Enumerating objects: 75, done. remote: Counting objects: 100% (75/75), done. remote: Compressing objects: 100% (12/12), done. remote: Total 77 (delta 65), reused 65 (delta 63), pack-reused 2 Unpacking objects: 100% (77/77), 34.07 KiB | 157.00 KiB/s, done. From git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip 161a9a33702a..cedd3614e5d9 perf/core -> tip/perf/core 6255b48aebfd..25795ef6299f sched/core -> tip/sched/core $ git rebase tip/x86/sgx Current branch master is up to date. BR, Jarkko
On 3/1/22 17:41, Jarkko Sakkinen wrote: > On Tue, Mar 01, 2022 at 09:54:14AM -0800, Dave Hansen wrote: >> On 3/1/22 04:58, Jarkko Sakkinen wrote: >>> @@ -32,14 +58,16 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, >>> else >>> page_index = PFN_DOWN(encl->size); >>> >>> + page_pcmd_off = sgx_encl_get_backing_page_pcmd_offset(encl, page_index); >>> + >>> ret = sgx_encl_lookup_backing(encl, page_index, &b); >>> if (ret) >>> return ret; >> What tree is this against? It looks like it might be on top of >> Kristen's overcommit series. >> >> It would be best if you could test this on top of tip/sgx. Kristen >> changed code in this area as well. > I rebased this against latest stuff and now I did a sanity check: > > $ git fetch tip > remote: Enumerating objects: 75, done. > remote: Counting objects: 100% (75/75), done. > remote: Compressing objects: 100% (12/12), done. > remote: Total 77 (delta 65), reused 65 (delta 63), pack-reused 2 > Unpacking objects: 100% (77/77), 34.07 KiB | 157.00 KiB/s, done. >>From git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip > 161a9a33702a..cedd3614e5d9 perf/core -> tip/perf/core > 6255b48aebfd..25795ef6299f sched/core -> tip/sched/core > > $ git rebase tip/x86/sgx > Current branch master is up to date. Are you sure you didn't also rebase all of Kristen's work along with your one patch? How many patches did you rebase? You might want to do: git log tip/x86/sgx..
On Wed, Mar 02, 2022 at 08:40:29AM -0800, Dave Hansen wrote: > On 3/1/22 17:41, Jarkko Sakkinen wrote: > > On Tue, Mar 01, 2022 at 09:54:14AM -0800, Dave Hansen wrote: > >> On 3/1/22 04:58, Jarkko Sakkinen wrote: > >>> @@ -32,14 +58,16 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > >>> else > >>> page_index = PFN_DOWN(encl->size); > >>> > >>> + page_pcmd_off = sgx_encl_get_backing_page_pcmd_offset(encl, page_index); > >>> + > >>> ret = sgx_encl_lookup_backing(encl, page_index, &b); > >>> if (ret) > >>> return ret; > >> What tree is this against? It looks like it might be on top of > >> Kristen's overcommit series. > >> > >> It would be best if you could test this on top of tip/sgx. Kristen > >> changed code in this area as well. > > I rebased this against latest stuff and now I did a sanity check: > > > > $ git fetch tip > > remote: Enumerating objects: 75, done. > > remote: Counting objects: 100% (75/75), done. > > remote: Compressing objects: 100% (12/12), done. > > remote: Total 77 (delta 65), reused 65 (delta 63), pack-reused 2 > > Unpacking objects: 100% (77/77), 34.07 KiB | 157.00 KiB/s, done. > >>From git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip > > 161a9a33702a..cedd3614e5d9 perf/core -> tip/perf/core > > 6255b48aebfd..25795ef6299f sched/core -> tip/sched/core > > > > $ git rebase tip/x86/sgx > > Current branch master is up to date. > > Are you sure you didn't also rebase all of Kristen's work along with > your one patch? How many patches did you rebase? > > You might want to do: > > git log tip/x86/sgx.. I did "git reset --hard tip/x86/sgx" and reapplied patch, and fixed the merge conflict, i.e. "lookup" to "get" :-) You're right. BR, Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 8be6f0592bdc..3d2ed8d27747 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -12,6 +12,30 @@ #include "encls.h" #include "sgx.h" +/* + * Calculate byte offset of a PCMD struct associated with an enclave page. PCMD's + * follow right after the EPC data in the backing storage. In addition to the + * visible enclave pages, there's one extra page slot for SECS, before PCMD + * structs. + */ +static inline pgoff_t sgx_encl_get_backing_page_pcmd_offset(struct sgx_encl *encl, + unsigned long page_index) +{ + pgoff_t epc_end_off = encl->size + sizeof(struct sgx_secs); + + return epc_end_off + page_index * sizeof(struct sgx_pcmd); +} + +/* + * Free a page from the backing storage in the given page index. + */ +static inline void sgx_encl_truncate_backing_page(struct sgx_encl *encl, unsigned long page_index) +{ + struct inode *inode = file_inode(encl->backing); + + shmem_truncate_range(inode, PFN_PHYS(page_index), PFN_PHYS(page_index) + PAGE_SIZE - 1); +} + /* * ELDU: Load an EPC page as unblocked. For more info, see "OS Management of EPC * Pages" in the SDM. @@ -22,9 +46,11 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, { unsigned long va_offset = encl_page->desc & SGX_ENCL_PAGE_VA_OFFSET_MASK; struct sgx_encl *encl = encl_page->encl; + pgoff_t page_index, page_pcmd_off; struct sgx_pageinfo pginfo; struct sgx_backing b; - pgoff_t page_index; + bool pcmd_page_empty; + u8 *pcmd_page; int ret; if (secs_page) @@ -32,14 +58,16 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, else page_index = PFN_DOWN(encl->size); + page_pcmd_off = sgx_encl_get_backing_page_pcmd_offset(encl, page_index); + ret = sgx_encl_lookup_backing(encl, page_index, &b); if (ret) return ret; pginfo.addr = encl_page->desc & PAGE_MASK; pginfo.contents = (unsigned long)kmap_atomic(b.contents); - pginfo.metadata = (unsigned long)kmap_atomic(b.pcmd) + - b.pcmd_offset; + pcmd_page = kmap_atomic(b.pcmd); + pginfo.metadata = (unsigned long)pcmd_page + b.pcmd_offset; if (secs_page) pginfo.secs = (u64)sgx_get_epc_virt_addr(secs_page); @@ -55,11 +83,24 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, ret = -EFAULT; } - kunmap_atomic((void *)(unsigned long)(pginfo.metadata - b.pcmd_offset)); + memset(pcmd_page + b.pcmd_offset, 0, sizeof(struct sgx_pcmd)); + + /* + * The area for the PCMD in the page was zeroed above. Check if the + * whole page is now empty meaning that all PCMD's have been zeroed: + */ + pcmd_page_empty = !memchr_inv(pcmd_page, 0, PAGE_SIZE); + + kunmap_atomic(pcmd_page); kunmap_atomic((void *)(unsigned long)pginfo.contents); sgx_encl_put_backing(&b, false); + sgx_encl_truncate_backing_page(encl, page_index); + + if (pcmd_page_empty) + sgx_encl_truncate_backing_page(encl, PFN_DOWN(page_pcmd_off)); + return ret; } @@ -583,7 +624,7 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, static 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); + pgoff_t page_pcmd_off = sgx_encl_get_backing_page_pcmd_offset(encl, page_index); struct page *contents; struct page *pcmd; @@ -591,7 +632,7 @@ static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, if (IS_ERR(contents)) return PTR_ERR(contents); - pcmd = sgx_encl_get_backing_page(encl, pcmd_index); + pcmd = sgx_encl_get_backing_page(encl, PFN_DOWN(page_pcmd_off)); if (IS_ERR(pcmd)) { put_page(contents); return PTR_ERR(pcmd); @@ -600,9 +641,7 @@ static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, 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); + backing->pcmd_offset = page_pcmd_off & (PAGE_SIZE - 1); return 0; }
There is a limited amount of SGX memory (EPC) on each system. When that memory is used up, SGX has its own swapping mechanism which is similar in concept but totally separate from the core mm/* code. Instead of swapping to disk, SGX swaps from EPC to normal RAM. That normal RAM comes from a shared memory pseudo-file and can itself be swapped by the core mm code. There is a hierarchy like this: EPC <-> shmem <-> disk After data is swapped back in from shmem to EPC, the shmem backing storage needs to be freed. Currently, the backing shmem is not freed. This effectively wastes the shmem while the enclave is running. The memory is recovered when the enclave is destroyed and the backing storage freed. Sort this out by freeing memory with shmem_truncate_range(), as soon as a page is faulted back to the EPC. In addition, free the memory for PCMD pages as soon as all PCMD's in a page have been marked as unused by zeroing its contents. Reported-by: Dave Hansen <dave.hansen@linux.intel.com> Cc: stable@vger.kernel.org Fixes: 1728ab54b4be ("x86/sgx: Add a page reclaimer") Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v5: * Encapsulated file offset calculation for PCMD struct. * Replaced "magic number" PAGE_SIZE with sizeof(struct sgx_secs) to make the offset calculation more self-documentative. v4: * Sanitized the offset calculations. v3: * Resend. v2: * Rewrite commit message as proposed by Dave. * Truncate PCMD pages (Dave). --- arch/x86/kernel/cpu/sgx/encl.c | 57 ++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 9 deletions(-)