Re: [PATCH v2] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Sat, Apr 04, 2020 at 02:33:31AM +0300, Jarkko Sakkinen wrote:
> On Fri, Apr 03, 2020 at 12:35:50PM +0300, Jarkko Sakkinen wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > -	idx = srcu_read_lock(&encl->srcu);
> > +	do {
> > +		mm_list_version = encl->mm_list_version;
> >  
> > -	list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) {
> > -		if (!mmget_not_zero(encl_mm->mm))
> > -			continue;
> > +		/* Fence reads as the CPU can reorder them. This guarantees
> > +		 * that we don't access old list with a new version.
> > +		 */
> > +		smp_rmb();
> >  
> > -		down_read(&encl_mm->mm->mmap_sem);
> > +		idx = srcu_read_lock(&encl->srcu);
> >  
> > -		ret = sgx_encl_find(encl_mm->mm, addr, &vma);
> > -		if (!ret && encl == vma->vm_private_data)
> > -			zap_vma_ptes(vma, addr, PAGE_SIZE);
> > +		list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) {
> > +			if (!mmget_not_zero(encl_mm->mm))
> > +				continue;
> >  
> > -		up_read(&encl_mm->mm->mmap_sem);
> > +			down_read(&encl_mm->mm->mmap_sem);
> >  
> > -		mmput_async(encl_mm->mm);
> > -	}
> > +			ret = sgx_encl_find(encl_mm->mm, addr, &vma);
> > +			if (!ret && encl == vma->vm_private_data)
> > +				zap_vma_ptes(vma, addr, PAGE_SIZE);
> >  
> > -	srcu_read_unlock(&encl->srcu, idx);
> > +			up_read(&encl_mm->mm->mmap_sem);
> > +
> > +			mmput_async(encl_mm->mm);
> > +		}
> > +
> > +		srcu_read_unlock(&encl->srcu, idx);
> > +	} while (unlikely(encl->mm_list_version != mm_list_version));
> 
> This is bad, or at least makes the code unnecessarily hard to understand
> given the use of the fences: encl->mm_list_version is read twice per
> iteration.
> 
> I'll change the code to use "for ( ; ; )" and have exactly one read per
> iteration.

It has to be read twice per iteration, the whole point is to see if the
version at the start of the iteration matches the version at the end.