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

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

On Fri, Apr 03, 2020 at 04:35:47PM -0700, Sean Christopherson wrote:
> 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.

Nope if you use zero version as invalid version.

/Jarkko