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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Dave,

On 5/10/2022 2:55 PM, Dave Hansen wrote:
> 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.

This function is called after a PCMD page is determined to be empty and is
about to be truncated. The question this function needs to answer is, "is this
PCMD page in use?" - that is, even though it is empty, it cannot be truncated
since there is a reference to this page (specifically from the reclaimer) and
a reference is obtained with the intent to write data to the page.

For some other name options, how about:
reclaimer_has_pcmd_ref()
reclaimer_using_pcmd()

Do any of these look better to you?

> 
>> + * @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.

Thank you. I will emphasize that.

> 
>> + * 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?

Yes, will do.

> 
>> +
>> +	/*
>> +	 * 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.

Thank you very much for catching this. This should be:

entry = xa_load(&encl->page_array, PFN_DOWN(addr));
if (!entry)
	continue;

> 
>> +		/*
>> +		 * 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?

I am not familiar with the history behind this. The VA slot information
do indeed take up quite a few bits though, leaving just three bits
available.

> 
>> +		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.

Will do. I will add a "Context:" portion to the function's kernel-doc.

Reinette