On Wed, Jun 19, 2019 at 11:22:16AM -0700, Sean Christopherson wrote: > Using per-vma refcounting to track mm_structs associated with an enclave > requires hooking .vm_close(), which in turn prevents the mm from merging > vmas (precisely to allow refcounting). > > Avoid refcounting encl_mm altogether by registering an mmu_notifier at > .mmap(), removing the dying encl_mm at mmu_notifier.release() and > protecting mm_list during reclaim via a per-enclave SRCU. > > Removing refcounting/vm_close() allows merging of enclave vmas, at the > cost of delaying removal of encl_mm structs from mm_list, i.e. an mm is > disassociated from an enclave when the mm exits or the enclave dies, as > opposed to when the last vma (in a given mm) is closed. > > The impact of delying encl_mm removal is its memory footprint and > whatever overhead is incurred during EPC reclaim (to walk an mm's vmas). > Practically speaking, a stale encl_mm will exist for a meaningful amount > of time if and only if the enclave is mapped in a long-lived process and > then passed off to another long-lived process. It is expected that the > vast majority of use cases will not encounter this condition, e.g. even > using a daemon to build enclaves should not result in a stale encl_mm as > the builder should never need to mmap() the enclave. > > Even if there are scenarios that lead to defunct encl_mms, the cost is > likely far outweighed by the benefits of reducing the number of vmas > across all enclaves. > > Note, using SRCU to protect mm_list is not strictly necessary, i.e. the > existing walker with encl_mm refcounting could be massaged to work with > mmu_notifier.release(), but the resulting code is subtle and fragile (I > never actually got it working). The primary issue is that an encl_mm > can't be moved off the list until its refcount goes to zero, otherwise > the custom walker goes off into the weeds. The refcount requirement > then prevents using mm_list to identify if an mmu_notifier.release() > has fired, i.e. another mechanism is needed to guard against races > between exit_mmap() and sgx_release(). > > Cc: Dave Hansen <dave.hansen@xxxxxxxxx> > Cc: Andy Lutomirski <luto@xxxxxxxxxx> > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> OK, so what this needs is to split two patches. That can be seen from your last paragraph (you are implying it with your note). Then this can be sanely reviewed. And no, this does not fight by any means with the changes that I'm doing. /Jarkko
