Re: [PATCH v30 08/20] x86/sgx: Add functions to allocate and free EPC pages

[Borislav Petkov]

On Fri, May 15, 2020 at 03:43:58AM +0300, Jarkko Sakkinen wrote:
> Add functions for allocating page from Enclave Page Cache (EPC). A page is

				pages

> allocated by going through the EPC sections and returning the first free
> page.
> 
> When a page is freed, it might have a valid state, which means that the
> callee has assigned it to an enclave, which are protected memory ares used

								  areas

although explaining what enclaves are has already happened so probably
not needed here too.

> to run code protected from outside access. The page is returned back to the
> invalid state with ENCLS[EREMOVE] [1].
> 
> [1] Intel SDM: 40.3 INTELÂ SGX SYSTEM LEAF FUNCTION REFERENCE
> 
> Co-developed-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Acked-by: Jethro Beekman <jethro@xxxxxxxxxxxx>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 60 ++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h  |  3 ++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 38424c1e8341..60d82e7537c8 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -13,6 +13,66 @@
>  struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
>  int sgx_nr_epc_sections;
>  
> +static struct sgx_epc_page *__sgx_try_alloc_page(struct sgx_epc_section *section)
> +{
> +	struct sgx_epc_page *page;
> +
> +	if (list_empty(&section->page_list))
> +		return NULL;
> +
> +	page = list_first_entry(&section->page_list, struct sgx_epc_page, list);
> +	list_del_init(&page->list);
> +	return page;
> +}
> +
> +/**
> + * sgx_try_alloc_page() - Allocate an EPC page

Uuh, this is confusing. Looking forward into the patchset, you guys have

sgx_alloc_page()
sgx_alloc_va_page()

and this here

sgx_try_alloc_page()

So this one here should be called sgx_alloc_epc_page() if we're going to
differentiate *what* it is allocating.

But then looking at sgx_alloc_page():

+ * sgx_alloc_page() - Allocate an EPC page
...

+ struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim)

this one returns a struct sgx_epc_page * too.

The former "allocates" from the EPC cache so I'm thinking former should
not have "alloc" in its name at all. It should be called something like

sgx_get_epc_page()

or so.

Now, looking at sgx_alloc_page(), it does call this one -
sgx_try_alloc_page() to get a page from the page list but it
does not allocate anything. The actual allocation happens in
sgx_alloc_epc_section() which actually does the k*alloc().

Which sounds to me like those functions should not use "alloc" and
"free" in their names but "get" and "put" to denote that they're simply
getting pages from lists and returning them back to those lists.

IMNSVHO.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette