On Mon, 22 Mar 2021 12:37:02 -0700 Sean Christopherson wrote: > On Mon, Mar 22, 2021, Borislav Petkov wrote: > > On Mon, Mar 22, 2021 at 11:56:37AM -0700, Sean Christopherson wrote: > > > 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. > > > > That's what I meant. Rebooting guests is a lot easier, ofc. > > > > Or are you saying, this can trigger *only* when they're running enclaves > > on the *host* too? > > Yes. Note, it's still true if you strike out the "too", KVM support is completely > orthogonal to this code. The purpose of this patch is to separate out the EREMOVE > path used for host enclaves (/dev/sgx_enclave), because EPC virtualization for > KVM will have non-buggy scenarios where EREMOVE can fail. But the virt EPC code > is designed to handle that gracefully. > > > > EREMOVE can only fail if there's a kernel or hardware bug (or a VMM bug if > > > running as a guest). > > > > We get those on a daily basis. > > > > > 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". > > > > So this sounds to me like this should BUG too eventually. > > > > Or is this one of those "this should never happen" things so no one > > should worry? > > Hmm. I don't think it warrants BUG. At worst, leaking EPC pages is fatal only > to SGX. If the underlying bug caused other fallout, e.g. didn't release a lock, > then obviously that could be fatal to the kernel. But I don't think there's > ever a case where SGX being unusuable would prevent the kernel from functioning. > > > Whatever it is, if an admin sees this message in dmesg and doesn't get a > > lengthy explanation what she/he is supposed to do, I don't think she/he > > will be as relaxed. > > > > Hell, people open bugs for correctable ECCs and are asking whether they > > need to replace their hardware. > > LOL. > > > So let's play this out: put yourself in an admin's shoes and tell me how > > should an admin react when she/he sees that? > > > > Should the kernel probably also say: "Don't worry, you have enough > > memory and what's a 4K, who cares? You'll reboot eventually." > > > Or should the kernel say "You need to reboot ASAP." > > > > And so on... > > > > So what is the scenario here and what kind of reaction is that message > > supposed to cause, recovery action, blabla, the whole spiel? > > Probably something in between. Odds are good SGX will eventually become > unusuable, e.g. either kernel SGX support is completely hosted, or it will soon > leak the majority of EPC pages. Something like this? > > "EREMOVE returned %d (0x%x), kernel bug likely. EPC page leaked, SGX may become unusuable. Reboot recommended to continue using SGX." Or perhaps just stick to old behavior in original sgx_free_epc_page()? ret = __eremove(sgx_get_epc_virt_addr(page)); if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) return; This code path is only used by host SGX driver, but not KVM. And this patch's *main* intention is to break EREMOVE out of sgx_free_epc_page() so virtual EPC code can use sgx_free_epc_page(). Improving the error msg can be a separate discussion and separate patch which can be done in the future, and this has nothing to do with SGX virtualization support.
