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.
