Re: [PATCH v3 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()

[Sean Christopherson]

On Mon, Mar 22, 2021, Borislav Petkov wrote:
> On Fri, Mar 19, 2021 at 08:22:19PM +1300, Kai Huang wrote:
> > +/**
> > + * sgx_encl_free_epc_page - free EPC page assigned to an enclave
> > + * @page:	EPC page to be freed
> > + *
> > + * Free EPC page assigned to an enclave.  It does EREMOVE for the page, and
> > + * only upon success, it puts the page back to free page list.  Otherwise, it
> > + * gives a WARNING to indicate page is leaked, and require reboot to retrieve
> > + * leaked pages.
> > + */
> > +void sgx_encl_free_epc_page(struct sgx_epc_page *page)
> > +{
> > +	int ret;
> > +
> > +	WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> > +
> > +	/*
> > +	 * Give a message to remind EPC page is leaked when EREMOVE fails,
> > +	 * and requires machine reboot to get leaked pages back. This can
> > +	 * be improved in future by adding stats of leaked pages, etc.
> > +	 */
> > +#define EREMOVE_ERROR_MESSAGE \
> > +	"EREMOVE returned %d (0x%x).  EPC page leaked.  Reboot required to retrieve leaked pages."
> 
> A reboot? Seriously? Why?
> 
> How are you going to explain to cloud people that they need to reboot
> their fat server? The same cloud people who want to make sure Intel
> supports late microcode loading no matter the effort just so to avoid
> rebooting the machine.
> 
> But now all of a sudden, if they wanna have SGX enclaves in guests, they
> need to get prepared for potential rebooting.

Not necessarily.  This can only trigger in the host, and thus require a host
reboot, if the host is also running enclaves.  If the CSP is not running
enclaves, or is running its enclaves in a separate VM, then this path cannot be
reached.

> I sure hope I'm missing something...

EREMOVE can only fail if there's a kernel or hardware bug (or a VMM bug if
running as a guest).  IME, nearly every kernel/KVM bug that I introduced that
led to EREMOVE failure was also quite fatal to SGX, i.e. this is just the canary
in the coal mine.

It's certainly possible to add more sophisticated error handling, e.g. through
the pages onto a list and periodically try to recover them.  But, since the vast
majority of bugs that cause EREMOVE failure are fatal to SGX, implementing
sophisticated handling is quite low on the list of priorities.

Dave wanted the "page leaked" error message so that it's abundantly clear that
the kernel is leaking pages on EREMOVE failure and that the WARN isn't "benign".