Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

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

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@intel.com>
> > > ---
> > >  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