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@xxxxxxxxx>
> > > - 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
