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

[Jarkko Sakkinen]

On Thu, Dec 07, 2017 at 02:46:39PM +0000, Christopherson, Sean J wrote:
> > +	for (i = 0; i < 2; i++) {
> > +		va_page = list_first_entry(&encl->va_pages,
> > +					   struct sgx_va_page, list);
> > +		va_offset = sgx_alloc_va_slot(va_page);
> > +		if (va_offset < PAGE_SIZE)
> > +			break;
> > +
> > +		list_move_tail(&va_page->list, &encl->va_pages);
> > +	}
> 
> This is broken, there is no guarantee that the next VA page will have
> a free slot.  You have to walk over all VA pages to guarantee a slot
> is found, e.g. this caused EWB and ELDU errors.

I did run some extensive stress tests on this and did not experience any
issues. Full VA pages are always put to the end. Please point me to the
test where this breaks so that I can fix the issue if it persists.

> Querying list.next to determine if an encl_page is resident in the EPC
> is ugly and unintuitive, and depending on list's internal state seems
> dangerous.  Why not use a flag in the encl_page, e.g. as in the patch
> I submitted almost 8 months ago for combining epc_page and va_page into
> a union?  And, the encl's SGX_ENCL_SECS_EVICTED flag can be dropped if
> a flag is added to indicate whether or not any encl_page is resident in
> the EPC.
> 
> https://lists.01.org/pipermail/intel-sgx-kernel-dev/2017-April/000570.html

I think it is better to just zero list entry and do list_empty test. You
correct that checking that with poison is ugly.

Last flag bit wll be needed for the SGX_ENCL_PAGE_TRIM. It is useful to
have the flag in the enclave in order to be able to pack struct
sgx_encl_page.

/Jarkko