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

[Christopherson, Sean J]

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);