| Message ID | bd228c90c139437bb4fcc4b7b99063bfd3eb1439.1638381245.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx and selftests/sgx: Support SGX2 | expand |
On Wed, Dec 01, 2021 at 11:23:05AM -0800, Reinette Chatre wrote: > The SGX reclaimer removes page table entries pointing to pages that are > moved to swap. SGX2 enables changes to pages belonging to an initialized > enclave, for example changing page permissions. Supporting SGX2 requires > this ability to remove page table entries that is available in the > SGX reclaimer code. Missing: why SGX2 requirest this? > Factor out the code removing page table entries to a separate function, > fixing accuracy of comments in the process, and make it available to other > areas within the SGX code. > > Since the code will no longer be unique to the reclaimer it is relocated > to be with the rest of the enclave code in encl.c interacting with the > page table. This last paragraph should be removed. It can be seen from the code change and diffstat. /Jarkko
Hi Jarkko, On 12/4/2021 2:59 PM, Jarkko Sakkinen wrote: > On Wed, Dec 01, 2021 at 11:23:05AM -0800, Reinette Chatre wrote: >> The SGX reclaimer removes page table entries pointing to pages that are >> moved to swap. SGX2 enables changes to pages belonging to an initialized >> enclave, for example changing page permissions. Supporting SGX2 requires >> this ability to remove page table entries that is available in the >> SGX reclaimer code. > > Missing: why SGX2 requirest this? The above paragraph states that SGX2 needs to remove page table entries because it modifies page permissions. Could you please elaborate what is missing? > >> Factor out the code removing page table entries to a separate function, >> fixing accuracy of comments in the process, and make it available to other >> areas within the SGX code. >> >> Since the code will no longer be unique to the reclaimer it is relocated >> to be with the rest of the enclave code in encl.c interacting with the >> page table. > > This last paragraph should be removed. It can be seen from the code change > and diffstat. I understand that the code movement can be seen from the diffstat but the reason for the move may not be obvious to everybody. If it is ok with you I'd rather keep this text. Reinette
On Mon, 2021-12-06 at 13:30 -0800, Reinette Chatre wrote: > Hi Jarkko, > > On 12/4/2021 2:59 PM, Jarkko Sakkinen wrote: > > On Wed, Dec 01, 2021 at 11:23:05AM -0800, Reinette Chatre wrote: > > > The SGX reclaimer removes page table entries pointing to pages that are > > > moved to swap. SGX2 enables changes to pages belonging to an initialized > > > enclave, for example changing page permissions. Supporting SGX2 requires > > > this ability to remove page table entries that is available in the > > > SGX reclaimer code. > > > > Missing: why SGX2 requirest this? > > The above paragraph states that SGX2 needs to remove page table entries > because it modifies page permissions. Could you please elaborate what is > missing? It does not say why SGX2 requires an ability to remove page table entries. /Jarkko
Hi Jarkko, On 12/10/2021 11:52 PM, Jarkko Sakkinen wrote: > On Mon, 2021-12-06 at 13:30 -0800, Reinette Chatre wrote: >> Hi Jarkko, >> >> On 12/4/2021 2:59 PM, Jarkko Sakkinen wrote: >>> On Wed, Dec 01, 2021 at 11:23:05AM -0800, Reinette Chatre wrote: >>>> The SGX reclaimer removes page table entries pointing to pages that are >>>> moved to swap. SGX2 enables changes to pages belonging to an initialized >>>> enclave, for example changing page permissions. Supporting SGX2 requires >>>> this ability to remove page table entries that is available in the >>>> SGX reclaimer code. >>> >>> Missing: why SGX2 requirest this? >> >> The above paragraph states that SGX2 needs to remove page table entries >> because it modifies page permissions. Could you please elaborate what is >> missing? > > It does not say why SGX2 requires an ability to remove page table entries. Are you saying that modification of EPCM page permissions is not a reason to remove page table entries pointing to those pages? Reinette
On Mon, Dec 13, 2021 at 02:11:26PM -0800, Reinette Chatre wrote: > Hi Jarkko, > > On 12/10/2021 11:52 PM, Jarkko Sakkinen wrote: > > On Mon, 2021-12-06 at 13:30 -0800, Reinette Chatre wrote: > > > Hi Jarkko, > > > > > > On 12/4/2021 2:59 PM, Jarkko Sakkinen wrote: > > > > On Wed, Dec 01, 2021 at 11:23:05AM -0800, Reinette Chatre wrote: > > > > > The SGX reclaimer removes page table entries pointing to pages that are > > > > > moved to swap. SGX2 enables changes to pages belonging to an initialized > > > > > enclave, for example changing page permissions. Supporting SGX2 requires > > > > > this ability to remove page table entries that is available in the > > > > > SGX reclaimer code. > > > > > > > > Missing: why SGX2 requirest this? > > > > > > The above paragraph states that SGX2 needs to remove page table entries > > > because it modifies page permissions. Could you please elaborate what is > > > missing? > > > > It does not say why SGX2 requires an ability to remove page table entries. > > Are you saying that modification of EPCM page permissions is not a reason to > remove page table entries pointing to those pages? So you have: "Supporting SGX2 requires this ability to remove page table entries that is available in the SGX reclaimer code" Just write down where you need this ability (briefly). /Jarkko
Hi Jarkko, On 12/28/2021 6:55 AM, Jarkko Sakkinen wrote: > On Mon, Dec 13, 2021 at 02:11:26PM -0800, Reinette Chatre wrote: >> Hi Jarkko, >> >> On 12/10/2021 11:52 PM, Jarkko Sakkinen wrote: >>> On Mon, 2021-12-06 at 13:30 -0800, Reinette Chatre wrote: >>>> Hi Jarkko, >>>> >>>> On 12/4/2021 2:59 PM, Jarkko Sakkinen wrote: >>>>> On Wed, Dec 01, 2021 at 11:23:05AM -0800, Reinette Chatre wrote: >>>>>> The SGX reclaimer removes page table entries pointing to pages that are >>>>>> moved to swap. SGX2 enables changes to pages belonging to an initialized >>>>>> enclave, for example changing page permissions. Supporting SGX2 requires >>>>>> this ability to remove page table entries that is available in the >>>>>> SGX reclaimer code. >>>>> >>>>> Missing: why SGX2 requirest this? >>>> >>>> The above paragraph states that SGX2 needs to remove page table entries >>>> because it modifies page permissions. Could you please elaborate what is >>>> missing? >>> >>> It does not say why SGX2 requires an ability to remove page table entries. >> >> Are you saying that modification of EPCM page permissions is not a reason to >> remove page table entries pointing to those pages? > > So you have: > > "Supporting SGX2 requires this ability to remove page table entries that is > available in the SGX reclaimer code" > > Just write down where you need this ability (briefly). Will do. I will expand the current permission changing text and also add the need for this ability when regular pages are changed to TCS pages. TCS pages may not be accessed by enclave code so when a regular page becomes a TCS page any page table entries pointing to it should be removed. Reinette
On Thu, Jan 06, 2022 at 09:46:35AM -0800, Reinette Chatre wrote: > Hi Jarkko, > > On 12/28/2021 6:55 AM, Jarkko Sakkinen wrote: > > On Mon, Dec 13, 2021 at 02:11:26PM -0800, Reinette Chatre wrote: > >> Hi Jarkko, > >> > >> On 12/10/2021 11:52 PM, Jarkko Sakkinen wrote: > >>> On Mon, 2021-12-06 at 13:30 -0800, Reinette Chatre wrote: > >>>> Hi Jarkko, > >>>> > >>>> On 12/4/2021 2:59 PM, Jarkko Sakkinen wrote: > >>>>> On Wed, Dec 01, 2021 at 11:23:05AM -0800, Reinette Chatre wrote: > >>>>>> The SGX reclaimer removes page table entries pointing to pages that are > >>>>>> moved to swap. SGX2 enables changes to pages belonging to an initialized > >>>>>> enclave, for example changing page permissions. Supporting SGX2 requires > >>>>>> this ability to remove page table entries that is available in the > >>>>>> SGX reclaimer code. > >>>>> > >>>>> Missing: why SGX2 requirest this? > >>>> > >>>> The above paragraph states that SGX2 needs to remove page table entries > >>>> because it modifies page permissions. Could you please elaborate what is > >>>> missing? > >>> > >>> It does not say why SGX2 requires an ability to remove page table entries. > >> > >> Are you saying that modification of EPCM page permissions is not a reason to > >> remove page table entries pointing to those pages? > > > > So you have: > > > > "Supporting SGX2 requires this ability to remove page table entries that is > > available in the SGX reclaimer code" > > > > Just write down where you need this ability (briefly). > > Will do. I will expand the current permission changing text and also add the need > for this ability when regular pages are changed to TCS pages. TCS pages may not > be accessed by enclave code so when a regular page becomes a TCS page any page > table entries pointing to it should be removed. Thank you, sounds good. BR, Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index c29e10541d12..ba39186d5a28 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -587,7 +587,7 @@ int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm) spin_lock(&encl->mm_lock); list_add_rcu(&encl_mm->list, &encl->mm_list); - /* Pairs with smp_rmb() in sgx_reclaimer_block(). */ + /* Pairs with smp_rmb() in sgx_zap_enclave_ptes(). */ smp_wmb(); encl->mm_list_version++; spin_unlock(&encl->mm_lock); @@ -776,6 +776,49 @@ int sgx_encl_test_and_clear_young(struct mm_struct *mm, return ret; } +/** + * sgx_zap_enclave_ptes - remove PTEs mapping the address from enclave + * @encl: the enclave + * @addr: page aligned pointer to single page for which PTEs will be removed + * + * Multiple VMAs may have an enclave page mapped. Remove the PTE mapping + * @addr from each VMA. Ensure that page fault handler is ready to handle + * new mappings of @addr before calling this function. + */ +void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr) +{ + unsigned long mm_list_version; + struct sgx_encl_mm *encl_mm; + struct vm_area_struct *vma; + int idx, ret; + + do { + mm_list_version = encl->mm_list_version; + + /* Pairs with smp_wmb() in sgx_encl_mm_add(). */ + smp_rmb(); + + idx = srcu_read_lock(&encl->srcu); + + list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) { + if (!mmget_not_zero(encl_mm->mm)) + continue; + + mmap_read_lock(encl_mm->mm); + + ret = sgx_encl_find(encl_mm->mm, addr, &vma); + if (!ret && encl == vma->vm_private_data) + zap_vma_ptes(vma, addr, PAGE_SIZE); + + mmap_read_unlock(encl_mm->mm); + + mmput_async(encl_mm->mm); + } + + srcu_read_unlock(&encl->srcu, idx); + } while (unlikely(encl->mm_list_version != mm_list_version)); +} + /** * sgx_alloc_va_page() - Allocate a Version Array (VA) page * diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h index becb68503baa..82e21088e68b 100644 --- a/arch/x86/kernel/cpu/sgx/encl.h +++ b/arch/x86/kernel/cpu/sgx/encl.h @@ -112,7 +112,7 @@ int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write); int sgx_encl_test_and_clear_young(struct mm_struct *mm, struct sgx_encl_page *page); - +void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr); struct sgx_epc_page *sgx_alloc_va_page(void); unsigned int sgx_alloc_va_slot(struct sgx_va_page *va_page); void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset); diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index e5be992897f4..9b96f4e0a17a 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -135,36 +135,9 @@ static void sgx_reclaimer_block(struct sgx_epc_page *epc_page) struct sgx_encl_page *page = epc_page->owner; unsigned long addr = page->desc & PAGE_MASK; struct sgx_encl *encl = page->encl; - unsigned long mm_list_version; - struct sgx_encl_mm *encl_mm; - struct vm_area_struct *vma; - int idx, ret; - - do { - mm_list_version = encl->mm_list_version; - - /* Pairs with smp_rmb() in sgx_encl_mm_add(). */ - smp_rmb(); - - idx = srcu_read_lock(&encl->srcu); - - list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) { - if (!mmget_not_zero(encl_mm->mm)) - continue; - - mmap_read_lock(encl_mm->mm); - - ret = sgx_encl_find(encl_mm->mm, addr, &vma); - if (!ret && encl == vma->vm_private_data) - zap_vma_ptes(vma, addr, PAGE_SIZE); - - mmap_read_unlock(encl_mm->mm); - - mmput_async(encl_mm->mm); - } + int ret; - srcu_read_unlock(&encl->srcu, idx); - } while (unlikely(encl->mm_list_version != mm_list_version)); + sgx_zap_enclave_ptes(encl, addr); mutex_lock(&encl->lock);
The SGX reclaimer removes page table entries pointing to pages that are moved to swap. SGX2 enables changes to pages belonging to an initialized enclave, for example changing page permissions. Supporting SGX2 requires this ability to remove page table entries that is available in the SGX reclaimer code. Factor out the code removing page table entries to a separate function, fixing accuracy of comments in the process, and make it available to other areas within the SGX code. Since the code will no longer be unique to the reclaimer it is relocated to be with the rest of the enclave code in encl.c interacting with the page table. Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 45 +++++++++++++++++++++++++++++++++- arch/x86/kernel/cpu/sgx/encl.h | 2 +- arch/x86/kernel/cpu/sgx/main.c | 31 ++--------------------- 3 files changed, 47 insertions(+), 31 deletions(-)