On Sun, Oct 18, 2020 at 08:03:11AM +0300, Jarkko Sakkinen wrote: > > > + mmap_read_lock(current->mm); > > > + mutex_lock(&encl->lock); > > > + > > > + /* > > > + * Insert prior to EADD in case of OOM. > > > > I wouldn't say OOM. Maybe: > > > > xa_insert() and EADD can both fail. But xa_insert() is easier > > to unwind so do it first. > > > > > EADD modifies MRENCLAVE, i.e. > > > > What is MRENCLAVE? > > The measurement stored in SECS. I'm wondering with xarray, is it > possible to preallocate entry without inserting anything? > > Then we could get rid of this unwind and also would not need to > take encl->lock in sgx_encl_may_map(). I'm still a bit confused with the unfamiliar Xarray API but I think I got it: 1. xa_insert() with a NULL entry reserves index and more importantly does the memory allocation. 2. xa_cmpxchg() with the enclave page, if EADD and EEXTEND's succceed. 3. xa_release() otherwise. This way sgx_encl_may_map() will never see a stale enclave page when it does the permission check, even if encl->lock is not taken. I mean right now I have to take both xas lock and enclave lock, which is terrible but this will take care of it. I will rewrite the comment to something more reasonable, once I've done this code change. The reason for doing insert first is that, if we get -ENOMEM after successful EADD and EEXTEND's we have a legit microarchitectural state but you cannot rollback a hash (MRENCLAVE), so game is anyway over because your data structures are not in sync. If -ENOMEM comes before, everything is still in sync and we don't have invalidate the enclave. /Jarkko
