On Tue, Jun 18, 2019 at 07:11:47AM -0700, Sean Christopherson wrote:
> On Mon, Jun 17, 2019 at 04:42:59PM -0700, Andy Lutomirski wrote:
> > On Mon, Jun 17, 2019 at 3:24 PM Sean Christopherson
> > <sean.j.christopherson@xxxxxxxxx> wrote:
> > >
> > > The enclave mm tracking is currently broken:
> > >
> > > - Adding current->mm during ECREATE is wrong as there is no guarantee
> > > that the current process has mmap()'d the enclave, i.e. there may
> > > never be an associated sgx_vma_close() to drop the encl_mm.
> > >
> > > - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
> > > only when splitting or duplicating a vma. If userspace performs a
> > > single mmap() on the enclave then SGX will fail to track the mm.
> > > This bug is partially hidden by tracking current->mm at ECREATE.
> > >
> > > Rework the tracking to get/add the mm at mmap(). A side effect of the
> > > bug fix is that sgx_vma_{open,close}() should never encounter a vma with
> > > an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
> > > cannot be found in either condition.
> > >
> >
> > It would be nifty if you could also kill .vm_close, since then VMAs
> > could be merged properly. Would this be straightforward?
>
> Hmm, we probably could move the mm tracking to f_op->{open,release}. The
Scratch that thought, pre-coffee brain was thinking there was a magical
file op that was called whenever a new reference to the file was acquired.
The only idea I can think of to avoid .vm_close() would be to not refcount
the mm at all, and instead register a mmu notifier at .mmap() and use
mmu_notifier.release() to remove the mm. The downside is that the mm
tracking would be sticky, i.e. once a process mmap()'d an enclave it would
forever be tracked by the enclave. Practically speaking, in the worst
case this would add a small amount of overhead to reclaiming from an
enclave that was temporarily mapped by multiple processes. So it might
be a viable option?
> downside to that approach is that EPC reclaim would unnecessarily walk the
> vmas for processes that have opened the enclave but not mapped any EPC
> pages. In the grand scheme, that's a minor issue and probably worth the
> tradeoff of vma merging.
>
> On the plus side, in addition to zapping ->close, I think it would allow
> for a simpler vma walking scheme. Maybe.
