Re: [PATCH v19 18/27] x86/sgx: Add swapping code to the core and SGX driver

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

> Yuck.  Definitely should look at using RCU list.  I think the whole
> function would boil down to:
> 
> 	list_for_each_entry_rcu(...) {
> 		down_read(&mm->mm->mmap_sem);
> 		ret = !sgx_encl_test_and_clear_young(next_mm->mm, page);
> 		up_read(&mm->mm->mmap_sem);
> 
> 		if (ret || (encl->flags & SGX_ENCL_DEAD))
> 			break;
> 	}
> 
> 	if (!ret || (encl->flags & SGX_ENCL_DEAD)) {
> 		mutex_lock(&encl->lock);
> 		page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> 		mutex_unlock(&encl->lock);
> 	}

But yuo cnot

> > +
> > +		down_read(&next_mm->mm->mmap_sem);
> > +		mutex_lock(&encl->lock);
> 
> Acquiring encl->lock just to check if its dead is a bit silly.
> 
> > +
> > +		if (encl->flags & SGX_ENCL_DEAD) {
> > +			page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> > +			ret = true;
> > +			goto out_stop;
> > +		}
> > +
> > +		ret = !sgx_encl_test_and_clear_young(next_mm->mm, page);
> > +		if (!ret)
> > +			goto out_stop;
> > +
> > +		mutex_unlock(&encl->lock);
> > +		up_read(&next_mm->mm->mmap_sem);
> > +	}
> > +
> > +	page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> 
> SGX_ENCL_PAGE_RECLAIMED needs to be while holding encl->lock.  Putting
> everything together, I think the function would boil down to:
> 
> 	list_for_each_entry_rcu(...) {
> 		if (encl->flags & SGX_ENCL_DEAD)
> 			break;
> 
> 		down_read(&mm->mm->mmap_sem);
> 		ret = !sgx_encl_test_and_clear_young(next_mm->mm, page);
> 		up_read(&mm->mm->mmap_sem);
> 
> 		if (!ret)
> 			return false;
> 	}
> 
> 	mutex_lock(&encl->lock);
> 	page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> 	mutex_unlock(&encl->lock);
> 
> 	return true;
> 
> > +	return true;
> > +out_stop:
> > +	mutex_unlock(&encl->lock);
> > +	up_read(&next_mm->mm->mmap_sem);
> > +	mmdrop(next_mm->mm);
> > +	kref_put(&next_mm->refcount, sgx_encl_release_mm);
> > +	return ret;
> > +}
> > +
> > +static void sgx_reclaimer_block(struct sgx_epc_page *epc_page)
> > +{
> > +	struct sgx_encl_page *page = epc_page->owner;
> > +	unsigned long addr = SGX_ENCL_PAGE_ADDR(page);
> > +	struct sgx_encl *encl = page->encl;
> > +	struct sgx_encl_mm *next_mm = NULL;
> > +	struct sgx_encl_mm *prev_mm = NULL;
> > +	struct vm_area_struct *vma;
> > +	int iter;
> > +	int ret;
> > +
> > +	while (true) {
> > +		next_mm = sgx_encl_next_mm(encl, prev_mm, &iter);
> > +		if (prev_mm) {
> > +			mmdrop(prev_mm->mm);
> > +			kref_put(&prev_mm->refcount, sgx_encl_release_mm);
> > +		}
> > +		prev_mm = next_mm;
> > +
> > +		if (iter == SGX_ENCL_MM_ITER_DONE)
> > +			break;
> > +
> > +		if (iter == SGX_ENCL_MM_ITER_RESTART)
> > +			continue;
> > +
> > +		down_read(&next_mm->mm->mmap_sem);
> > +		mutex_lock(&encl->lock);
> 
> There's no need to acquire encl->lock, only mmap_sem needs to be held
> to zap PTEs.
> 
> > +		ret = sgx_encl_find(next_mm->mm, addr, &vma);
> > +		if (!ret && encl == vma->vm_private_data)
> > +			zap_vma_ptes(vma, addr, PAGE_SIZE);
> > +
> > +		mutex_unlock(&encl->lock);
> > +		up_read(&next_mm->mm->mmap_sem);
> > +	}
> > +
> > +	mutex_lock(&encl->lock);
> > +
> > +	if (!(encl->flags & SGX_ENCL_DEAD)) {
> > +		ret = __eblock(sgx_epc_addr(epc_page));
> > +		if (encls_failed(ret))
> > +			ENCLS_WARN(ret, "EBLOCK");
> > +	}
> > +
> > +	mutex_unlock(&encl->lock);
> > +}
> > +
> > +static int __sgx_encl_ewb(struct sgx_encl *encl, struct sgx_epc_page *epc_page,
> > +			  struct sgx_va_page *va_page, unsigned int va_offset)
> > +{
> > +	struct sgx_encl_page *encl_page = epc_page->owner;
> > +	pgoff_t page_index = sgx_encl_get_index(encl, encl_page);
> > +	pgoff_t pcmd_index = sgx_pcmd_index(encl, page_index);
> > +	unsigned long pcmd_offset = sgx_pcmd_offset(page_index);
> > +	struct sgx_pageinfo pginfo;
> > +	struct page *backing;
> > +	struct page *pcmd;
> > +	int ret;
> > +
> > +	backing = sgx_encl_get_backing_page(encl, page_index);
> > +	if (IS_ERR(backing)) {
> > +		ret = PTR_ERR(backing);
> > +		goto err_backing;
> > +	}
> > +
> > +	pcmd = sgx_encl_get_backing_page(encl, pcmd_index);
> > +	if (IS_ERR(pcmd)) {
> > +		ret = PTR_ERR(pcmd);
> > +		goto err_pcmd;
> > +	}
> > +
> > +	pginfo.addr = 0;
> > +	pginfo.contents = (unsigned long)kmap_atomic(backing);
> > +	pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset;
> > +	pginfo.secs = 0;
> > +	ret = __ewb(&pginfo, sgx_epc_addr(epc_page),
> > +		    sgx_epc_addr(va_page->epc_page) + va_offset);
> > +	kunmap_atomic((void *)(unsigned long)(pginfo.metadata - pcmd_offset));
> > +	kunmap_atomic((void *)(unsigned long)pginfo.contents);
> > +
> > +	set_page_dirty(pcmd);
> > +	put_page(pcmd);
> > +	set_page_dirty(backing);
> > +
> > +err_pcmd:
> > +	put_page(backing);
> > +
> > +err_backing:
> > +	return ret;
> > +}
> > +
> > +static void sgx_ipi_cb(void *info)
> > +{
> > +}
> > +
> > +static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
> > +{
> > +	struct sgx_encl_page *encl_page = epc_page->owner;
> > +	struct sgx_encl *encl = encl_page->encl;
> > +	struct sgx_encl_mm *next_mm = NULL;
> > +	struct sgx_encl_mm *prev_mm = NULL;
> > +	struct sgx_va_page *va_page;
> > +	unsigned int va_offset;
> > +	int iter;
> > +	int ret;
> > +
> > +	cpumask_clear(&encl->cpumask);
> > +
> > +	while (true) {
> > +		next_mm = sgx_encl_next_mm(encl, prev_mm, &iter);
> > +		if (prev_mm) {
> > +			mmdrop(prev_mm->mm);
> > +			kref_put(&prev_mm->refcount, sgx_encl_release_mm);
> > +		}
> > +		prev_mm = next_mm;
> > +
> > +		if (iter == SGX_ENCL_MM_ITER_DONE)
> > +			break;
> > +
> > +		if (iter == SGX_ENCL_MM_ITER_RESTART)
> > +			continue;
> > +
> > +		cpumask_or(&encl->cpumask, &encl->cpumask,
> > +			   mm_cpumask(next_mm->mm));
> > +	}
> 
> Sending IPIs to flush CPUs out of the enclave is only necessary if the
> enclave is alive, untracked and there are threads actively running in
> the enclave.  I.e. calculate cpumask only when necessary.
> 
> This open coding of IPI sending made me realize the driver no long
> invalidates an enclave if an ENCLS instruction fails unexpectedly.  That
> is going to lead to absolute carnage if something does go wrong as there
> will be no recovery path, i.e. the kernel log will be spammed to death
> with ENCLS WARNings.  Debugging future development will be a nightmare if
> a single ENCLS bug obliterates the kernel.

Responding below. I get your RCU idea but you cannot sleep inside normal
RCU. Also, the current implemntation deals with that mmap_sem cn be
gone. I'm open for using RCU (i.e. SRCU) if these can be somehow dealt
with.

/Jarkko