>
> >
> > Reading below, it could also fail due to running out of entropy, which is
> > still
> > legally possible to happen IMHO.
>
> Actually, no in this case, we only return false from sgx_updatesvn in case
> unknown
> error happens as agreed previously. In case we run out of entropy it should
> be safe
> to retry later and we dont prevent per current code EPC page allocation.
>
> >
> > Maybe just:
> > /*
> > * Updating SVN failed. SGX might be broken,
> > * or running out of entropy happened. Do
> > not
> > * allocate EPC page since it is not safe to
> > use
> > * SGX anymore if it was the former. If it was
> > * due to running out of entropy, the further
> > * call of EPC allocation will try
> > * sgx_updatesvn() again.
> > */
>
> I agree with this except that the current code doesn’t prevent EPC allocation
> on any
> other error return than unknown error. The question is whenever we want to
> change the behaviour to require it?
[...]
> > > + * Return:
> > > + * True: success or EUPDATESVN can be safely retried next time
> > > + * False: Unexpected error occurred
> >
> > Hmm.. IIUC it could fail with running out of entropy but this is still
> > legally
> > possible to happen. And it is safe to retry.
>
> Yes, in this case we get back SGX_INSUFFICIENT_ENTROPY currently we
> return "true" here and do not prevent EPC allocations of the page in this
> case, which means we will start populate EPC and can next time retry only
> when EPC is empty again.
[...]
> > > + switch (ret) {
> > > + case 0:
> > > + pr_info("EUPDATESVN: success\n");
> > > + break;
> > > + case SGX_EPC_NOT_READY:
> > > + case SGX_INSUFFICIENT_ENTROPY:
> > > + case SGX_EPC_PAGE_CONFLICT:
> > > + ENCLS_WARN(ret, "EUPDATESVN");
> > > + break;
> >
> > I don't think we should use ENCLS_WARN() for SGX_INSUFFICIENT_ENTROPY,
> > since
> > IIUC it is still legally possible to happen after the above retry.
> >
> > Also, it doesn't seem SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT
> > are needed
> > since IIUC the only possible error is out of entropy.
>
> Well, in case we have a kernel bug, and we think EPC is empty and it is safe
> to execute EUPDATESVN, while it is not the case, we can still get back the
> SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT from HW, right?
Right, but as you said, in case we have a kernel bug.
Which means it is not expected and we should just use ENCLS_WARN() for them.
IMHO we can even add
WARN_ON_ONCE(atomic_long_read(&sgx_nr_used_pages) != 0);
to sgx_updatesvn(), e.g., right after
lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
to assert sgx_updatesvn() is called when EPC is empty, thus SGX_EPC_NOT_READY
and SGX_EPC_PAGE_CONFLICT are not possible to happen.
> Which means we probably should warn about such buggy cases.
Yes.
> And maybe we should also prevent page allocation from EPC in this case also
> similarly as for unknown error?
Yes.
I don't see any reason we should continue to allow SGX to work in case of
SGX_EPC_NOT_READY or SGX_EPC_PAGE_CONFLICT.
IIUC, we also agreed in the last round discussion that we should:
"I guess the best action would be make sgx_alloc_epc_page() return
consistently -ENOMEM, if the unexpected happens."
SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT are indeed unexpected to me.
So my suggestion would be:
I think the sgx_updatesvn() should just return true when EUPDATESVN returns 0 or
SGX_NO_UPDATE, and return false for all other error codes. And it should
ENCLS_WARN() for all other error codes, except SGX_INSUFFICIENT_ENTROPY because
it can still legally happen.
Something like:
do {
ret = __eupdatesvn();
if (ret != SGX_INSUFFICIENT_ENTROPY)
break;
} while (--retry);
if (!ret || ret == SGX_NO_UPDATE) {
/*
* SVN successfully updated, or it was already up-to-date.
* Let users know when the update was successful.
*/
if (!ret)
pr_info("SVN updated successfully\n");
return true;
}
/*
* EUPDATESVN was called when EPC is empty, all other error
* codes are unexcepted except running out of entropy.
*/
if (ret != SGX_INSUFFICIENT_ENTROPY)
ENCLS_WARN(ret, "EUPDATESVN");
return false;
In __sgx_alloc_epc_page_from_node(), it should fail to allocate EPC page and
return -ENOMEM when sgx_updatesvn() returns false. We should only allow EPC to
be allocated when we know the SVN is already up-to-date.
Any further call of EPC allocation will trigger sgx_updatesvn() again. If it
was failed due to unexpected error, then it should continue to fail,
guaranteeing "sgx_alloc_epc_page() return consistently -ENOMEM, if the
unexpected happens". If it was failed due to running out of entropy, it then
may fail again, or it will just succeed and then SGX can continue to work.
