Re: [PATCH V2 4/5] x86/sgx: Fix race between reclaimer and page fault handler

[Dave Hansen <dave.hansen@intel.com>]

On 5/9/22 14:48, Reinette Chatre wrote:
...
> +#define PCMDS_PER_PAGE (PAGE_SIZE / sizeof(struct sgx_pcmd))
> +/*
> + * 32 PCMD entries share a PCMD page. PCMD_FIRST_MASK is used to
> + * determine the page index associated with the first PCMD entry
> + * within a PCMD page.
> + */
> +#define PCMD_FIRST_MASK GENMASK(4, 0)
> +
> +/**
> + * pcmd_page_in_use() - Query if any enclave page associated with a PCMD
> + *                      page is in process of being reclaimed.

The name is a bit too generic for what it does.

> + * @encl:        Enclave to which PCMD page belongs
> + * @start_addr:  Address of enclave page using first entry within the PCMD page
> + *
> + * When an enclave page is reclaimed some Paging Crypto MetaData (PCMD) is
> + * stored. The PCMD data of a reclaimed enclave page contains enough
> + * information for the processor to verify the page at the time
> + * it is loaded back into the Enclave Page Cache (EPC).
> + *
> + * The backing storage to which enclave pages are reclaimed is laid out as
> + * follows:
> + * All enclave pages:SECS page:PCMD pages
> + *
> + * Each PCMD page contains the PCMD metadata of
> + * PAGE_SIZE/sizeof(struct sgx_pcmd) enclave pages.
> + *
> + * A PCMD page can only be truncated if it is (a) empty, and (b) no enclave
> + * page sharing the PCMD page is in the process of being reclaimed.

Let's also point out that (b) is _because_ the page is about to become
non-empty.

> + * The reclaimer sets the SGX_ENCL_PAGE_BEING_RECLAIMED flag when it
> + * intends to reclaim that enclave page - it means that the PCMD data
> + * associated with that enclave page is about to get some data and thus
> + * even if the PCMD page is empty, it should not be truncated.
> + *
> + * Return: 1 the PCMD page is in use, 0 if is not in use, -EFAULT on error
> + */
> +static int pcmd_page_in_use(struct sgx_encl *encl,
> +			    unsigned long start_addr)
> +{
> +	struct sgx_encl_page *entry;
> +	unsigned long addr;
> +	int reclaimed = 0;
> +	int i;

Can 'addr' and 'entry' be declared within the loop instead?

> +
> +	/*
> +	 * PCMD_FIRST_MASK is based on number of PCMD entries within
> +	 * PCMD page being 32.
> +	 */
> +	BUILD_BUG_ON(PCMDS_PER_PAGE != 32);
> +
> +	for (i = 0; i < PCMDS_PER_PAGE; i++) {
> +		addr = start_addr + i * PAGE_SIZE;
> +
> +		/*
> +		 * Stop when reaching the SECS page - it does not
> +		 * have a page_array entry and its reclaim is
> +		 * started and completed with enclave mutex held so
> +		 * it does not use the SGX_ENCL_PAGE_BEING_RECLAIMED
> +		 * flag.
> +		 */
> +		if (addr == encl->base + encl->size)
> +			break;
> +
> +		entry = xa_load(&encl->page_array, PFN_DOWN(addr));
> +		if (!entry)
> +			return -EFAULT;

Can xa_load() return NULL if it simply can't find an 'encl_page' at
'addr'?  In that case, there would never be a PCMD entry for the page
since it doesn't exist.  Returning -EFAULT would be a
pcmd_page_in_use()==true condition, which doesn't seem accurate.

> +		/*
> +		 * VA page slot ID uses same bit as the flag so it is important
> +		 * to ensure that the page is not already in backing store.
> +		 */

Probably a patch for another day, but isn't this a bit silly?  Are we
short of bits in ->desc or something?

> +		if (entry->epc_page &&
> +		    (entry->desc & SGX_ENCL_PAGE_BEING_RECLAIMED)) {
> +			reclaimed = 1;
> +			break;
> +		}
> +	}
> +
> +	return reclaimed;
> +}

Could we also please add a comment about encl->lock needing to be held
over this?  Without that, there would be all kinds of trouble.