On Wed, 2022-04-13 at 14:10 -0700, Reinette Chatre wrote:
> struct sgx_encl should be protected with the mutex
> sgx_encl->lock. One exception is sgx_encl->page_cnt that
> is incremented (in sgx_encl_grow()) when an enclave page
> is added to the enclave. The reason the mutex is not held
> is to allow the reclaimer to be called directly if there are
> no EPC pages (in support of a new VA page) available at the time.
>
> Incrementing sgx_encl->page_cnt without sgc_encl->lock held
> is currently (before SGX2) safe from concurrent updates because
> all paths in which sgx_encl_grow() is called occur before
> enclave initialization and are protected with an atomic
> operation on SGX_ENCL_IOCTL.
>
> SGX2 includes support for dynamically adding pages after
> enclave initialization where the protection of SGX_ENCL_IOCTL
> is not available.
>
> Make direct reclaim of EPC pages optional when new VA pages
> are added to the enclave. Essentially the existing "reclaim"
> flag used when regular EPC pages are added to an enclave
> becomes available to the caller when used to allocate VA pages
> instead of always being "true".
>
> When adding pages without invoking the reclaimer it is possible
> to do so with sgx_encl->lock held, gaining its protection against
> concurrent updates to sgx_encl->page_cnt after enclave
> initialization.
>
> No functional change.
>
> Reported-by: Haitao Huang <haitao.huang@xxxxxxxxx>
> Tested-by: Haitao Huang <haitao.huang@xxxxxxxxx>
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
Reviewed-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
Nit: I don't think tested-by is in the right patch here. Maybe
Haitao's tested-by should be moved into patch that actually adds
support for EAUG? Not something I would NAK this patch, just
wondering...
> ---
> Changes since V3:
> - New patch prompted by Haitao encountering the
> WARN_ON_ONCE(encl->page_cnt % SGX_VA_SLOT_COUNT)
> within sgx_encl_grow() during his SGX2 multi-threaded
> unit tests.
>
> arch/x86/kernel/cpu/sgx/encl.c | 6 ++++--
> arch/x86/kernel/cpu/sgx/encl.h | 4 ++--
> arch/x86/kernel/cpu/sgx/ioctl.c | 8 ++++----
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 546423753e4c..92516aeca405 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -869,6 +869,8 @@ void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr)
>
> /**
> * sgx_alloc_va_page() - Allocate a Version Array (VA) page
> + * @reclaim: Reclaim EPC pages directly if none available. Enclave
> + * mutex should not be held if this is set.
> *
> * Allocate a free EPC page and convert it to a Version Array (VA) page.
> *
> @@ -876,12 +878,12 @@ void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr)
> * a VA page,
> * -errno otherwise
> */
> -struct sgx_epc_page *sgx_alloc_va_page(void)
> +struct sgx_epc_page *sgx_alloc_va_page(bool reclaim)
> {
> struct sgx_epc_page *epc_page;
> int ret;
>
> - epc_page = sgx_alloc_epc_page(NULL, true);
> + epc_page = sgx_alloc_epc_page(NULL, reclaim);
> if (IS_ERR(epc_page))
> return ERR_CAST(epc_page);
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index 253ebdd1c5be..66adb8faec45 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -116,14 +116,14 @@ struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
> unsigned long offset,
> u64 secinfo_flags);
> void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr);
> -struct sgx_epc_page *sgx_alloc_va_page(void);
> +struct sgx_epc_page *sgx_alloc_va_page(bool reclaim);
> 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);
> bool sgx_va_page_full(struct sgx_va_page *va_page);
> void sgx_encl_free_epc_page(struct sgx_epc_page *page);
> struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> unsigned long addr);
> -struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl);
> +struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl, bool reclaim);
> void sgx_encl_shrink(struct sgx_encl *encl, struct sgx_va_page *va_page);
>
> #endif /* _X86_ENCL_H */
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index bb8cdb2ad0d1..5d41aa204761 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -17,7 +17,7 @@
> #include "encl.h"
> #include "encls.h"
>
> -struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
> +struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl, bool reclaim)
> {
> struct sgx_va_page *va_page = NULL;
> void *err;
> @@ -30,7 +30,7 @@ struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
> if (!va_page)
> return ERR_PTR(-ENOMEM);
>
> - va_page->epc_page = sgx_alloc_va_page();
> + va_page->epc_page = sgx_alloc_va_page(reclaim);
> if (IS_ERR(va_page->epc_page)) {
> err = ERR_CAST(va_page->epc_page);
> kfree(va_page);
> @@ -64,7 +64,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> struct file *backing;
> long ret;
>
> - va_page = sgx_encl_grow(encl);
> + va_page = sgx_encl_grow(encl, true);
> if (IS_ERR(va_page))
> return PTR_ERR(va_page);
> else if (va_page)
> @@ -275,7 +275,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> return PTR_ERR(epc_page);
> }
>
> - va_page = sgx_encl_grow(encl);
> + va_page = sgx_encl_grow(encl, true);
> if (IS_ERR(va_page)) {
> ret = PTR_ERR(va_page);
> goto err_out_free;
BR, Jarkko
