On Wed, Jun 19, 2019 at 04:00:14PM +0300, Jarkko Sakkinen wrote:
> On Wed, 2019-06-19 at 15:56 +0300, Jarkko Sakkinen wrote:
> > On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson 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.
> > >
> > > Change the WARN() on a non-empty mm_list to a WARN_ONCE(). The warning
> > > will fire over and over (and over) if the mm tracking is broken, which
> > > hampers debug/triage far more than it helps.
> > >
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > ---
> > > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 ----------
> > > arch/x86/kernel/cpu/sgx/driver/main.c | 26 ++++++++++++++++++
> > > arch/x86/kernel/cpu/sgx/encl.c | 38 +++++---------------------
> > > arch/x86/kernel/cpu/sgx/encl.h | 4 +--
> > > 4 files changed, 35 insertions(+), 47 deletions(-)
> >
> > BTW, sgx_encl_mm is a bit confusing name (which I made) to begin with.
> > It clues as it was a some kind of 1:1 association with the process,
> > which it isn't.
> >
> > Could you update the patch and rename it as sgx_encl_mapping? After that
> > I'm happy to merge.
>
> And send it as a separate patch. Can merge it today/tomorrow after I've
> finished my reaper change.
Still working/testing reaper stuff but this one is now applied. Lets
not do any renames for the moment because they only slow down. That
was a bad suggestion from my part.
The reason I squashed this change is that it is a pure bug fix.
If there is a bug, lets try to just fix that with absolutely minimal
intrusion. Only after that consider a "better way".
/Jarkko
