Re: [intel-sgx-kernel-dev] [PATCH v7 4/8] intel_sgx: driver for Intel Software Guard Extensions

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Thu, Dec 14, 2017 at 09:36:05PM +0000, Christopherson, Sean J wrote:
> On Thu, Dec 14, 2017 at 03:10:06PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Dec 12, 2017 at 01:46:48PM -0800, Sean Christopherson wrote:
> > > So it looks like you avoid the described case by moving B to the head of
> > > the list in sgx_eldu.  The bug I am seeing is still straightforward to
> > > theorize:
> > >
> > >     1. Three VA pages.  List = A->B->C
> > >     2. Fill A and B, use one entry in C.  List = C->B->A
> > >     3. ELDU, freeing a slot in B.  List = B->C->A
> > >     4. EWB, consuming the last slot in B.  List = B->C->A
> > >     5. ELDU, freeing a slot in A.  List = A->B->C
> > >     6. EWB, consuming the last slot in A.  List = A->B->C
> > >     7. ELDU, but both A and B are full
> > >     8. Explode
> >
> > I see. It is easy to fix by moving back to of the list immediately after
> > last allocation. Thanks for pointing this out.
> 
> Why not keep it simple and iterate over all VA pages?  You can still
> move full pages to the back of the list to reduce the number of times
> full pages are queried.  IMO, juggling the pages on every EWB/ELDU
> adds complexity for little to no gain; there's no guarantee that the
> cache/TLB benefits of reusing a VA slot justifies the potential for
> thrashing the list, e.g. moving a previously-full VA page to the head
> of the list on ELDU will cause that page to get bounced back to the
> end of the list on the next EWB.  Besides, whatever performance might
> be gained is a drop in the bucket compared to the performance hit of
> evicting enough EPC pages to fill multiple VA pages.
> 
> e.g.
> 
> 	list_for_each_entry_safe(va_page, tmp, &encl->va_pages, list) {
> 		va_offset = sgx_alloc_va_slot(va_page);
> 		if (va_offset < PAGE_SIZE)
> 			break;
> 
> 		list_move_tail(&va_page->list, &full_pages);
> 	}
> 	list_splice_tail(&full_pages, &va_page->list);

It is easy to just to check whether VA page is full and move it back
of the list if it is.

/Jarkko