On Tue, 18 Jul 2023 09:30:11 -0500, Dave Hansen <dave.hansen@xxxxxxxxx>
wrote:
On 7/17/23 13:29, Haitao Huang wrote:
...
@@ -248,11 +258,9 @@ static struct sgx_encl_page
*__sgx_encl_load_page(struct sgx_encl *encl,
return entry;
}
- if (!(encl->secs.epc_page)) {
- epc_page = sgx_encl_eldu(&encl->secs, NULL);
- if (IS_ERR(epc_page))
- return ERR_CAST(epc_page);
- }
+ epc_page = sgx_encl_load_secs(encl);
+ if (IS_ERR(epc_page))
+ return ERR_CAST(epc_page);
epc_page = sgx_encl_eldu(entry, encl->secs.epc_page);
if (IS_ERR(epc_page))
@@ -339,6 +347,13 @@ static vm_fault_t sgx_encl_eaug_page(struct
vm_area_struct *vma,
mutex_lock(&encl->lock);
+ epc_page = sgx_encl_load_secs(encl);
+ if (IS_ERR(epc_page)) {
+ if (PTR_ERR(epc_page) == -EBUSY)
+ vmret = VM_FAULT_NOPAGE;
+ goto err_out_unlock;
+ }
Whenever I see one of these "make sure it isn't NULL", I always jump to
asking what *keeps* it from becoming NULL again. In both cases here, I
think that's encl->lock.
Yes, encl->lock protects all enclave states, the xarray holding
encl_pages, SECS, VAs, etc.
A comment would be really nice here, maybe on sgx_encl_load_secs().
Maybe:
/*
* Ensure the SECS page is not swapped out. Must be called with
* encl->lock to protect _____ and ensure the SECS page is not
* swapped out again.
*/
Thanks for the suggestion. Lock should be held for the duration of SECS
usage.
So something like this?
/*
* Ensure the SECS page is not swapped out. Must be called with
* encl->lock to protect the enclave states including SECS and
* ensure the SECS page is not swapped out again while being used.
*/
diff --git a/arch/x86/kernel/cpu/sgx/main.c
b/arch/x86/kernel/cpu/sgx/main.c
index 166692f2d501..4662a364ce62 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -257,6 +257,10 @@ static void sgx_reclaimer_write(struct
sgx_epc_page *epc_page,
mutex_lock(&encl->lock);
+ /* Should not be possible */
+ if (WARN_ON(!(encl->secs.epc_page)))
+ goto out;
That comment isn't super helpful. We generally don't WARN_ON() things
that should happen. *Why* is it not possible?
When this part of code is reached, the reclaimer is holding at least one
reclaimable EPC page to reclaim for the enclave and the code below only
reclaims SECS when no reclaimable EPCs (number of SECS children being
zero) of the enclave left. So it should not be possible.
I'll remove this change because this is really not needed for fixing the
bug as Kai pointed out.
I added this for sanity check when implementing multiple EPC tracking
lists for cgroups. At one point there were list corruption issues if
moving EPCs between lists not managed well. With those straightened out,
and clear definitions of EPC states for moving them from one list to
another, I no longer see much value to keep this even in later cgroup
patches.
Thanks
Haitao
