Re: [PATCH v3] x86/sgx: Free backing memory after faulting the enclave page

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

On 1/26/22 23:01, Jarkko Sakkinen wrote:
> On Sat, Jan 08, 2022 at 04:05:10PM +0200, Jarkko Sakkinen wrote:
>> +static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl *encl, pgoff_t index)
>> +{
>> +	return PFN_DOWN(encl->size) + 1 + (index / sizeof(struct sgx_pcmd));
>> +}
> Found it.
> 
> This should be 
> 
> static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl *encl, pgoff_t index)
> {
> 	return PFN_DOWN(encl->size) + 1 + (index * PAGE_SIZE) / sizeof(struct sgx_pcmd);
> }

I've looked at this for about 10 minutes and been simultaneously
confused as to whether it is right or wrong.  That makes it
automatically wrong. :)

First, this isn't calculating a "PCMD number".  It's calculating backing
offset.  The "PCMD number" calculation is only a part of it.  I think
that makes the naming rather sloppy.

Second, I think the typing is sloppy.  page_index for example:

> static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
>                            struct sgx_epc_page *epc_page,
>                            struct sgx_epc_page *secs_page)
> {
...
>         pgoff_t page_index;

It's storing a page number:

                page_index = PFN_DOWN(encl->size);

not a real offset-into-a-file.  That makes it even more confusing when
'page_index' crosses a function boundary, gets renamed to 'index' and
then its units get confused.

/*
 * Given a page number within an enclave (@epc_page_nr), calculate the
 * offset in bytes into the backing file where that page's PCMD is
 * located.
 */
-static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl
*encl, pgoff_t index)
+static inline pgoff_t sgx_page_nr_to_pcmd_nr(struct sgx_encl *encl,
unsigned long epc_page_nr)
{
	pgoff_t last_epc_offset = PFN_DOWN(encl->size);
	pgoff_t pcmd_offset;

	// The SECS page is stashed in a slot after the
	// last normal EPC page.  Leave space for it:
	last_epc_offset++;

	pcmd_offset = epc_page_nr / sizeof(struct sgx_pcmd);

	return last_epc_offset + pcmd_offset;
}

Looking at that, I still think your *original* version is correct.

Am I just all twisted around from looking at this code too much?  Could
you please take another look and send a new version of the patch?