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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 31, 2022 at 03:30:52PM -0800, Dave Hansen wrote:
> 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?

I gave some though and this is what I came up with.

So what do you think about this:

	pgoff_t pcmd_off = encl->size + PAGE_SIZE + page_idex * sizeof(struct sgx_pcmd); 
	
This makes the calculations quite simple:

	backing->pcmd = pcmd;
	backing->pcmd_offset = pcmd_off & PAGE_SIZE;

and

	pcmd = sgx_encl_get_backing_page(encl, PFN_DOWN(pcmd_off));
	
Then pgoff_t is actually also used for what it is meant to be.

/Jarkko



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux