From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org,
Bill Roberts <william.c.roberts@intel.com>,
Casey Schaufler <casey.schaufler@intel.com>,
James Morris <jmorris@namei.org>,
Dave Hansen <dave.hansen@intel.com>,
Cedric Xing <cedric.xing@intel.com>,
Andy Lutomirski <luto@kernel.org>,
Jethro Beekman <jethro@fortanix.com>,
"Dr . Greg Wettstein" <greg@enjellic.com>,
Stephen Smalley <sds@tycho.nsa.gov>
Subject: Re: [RFC PATCH v4 01/12] x86/sgx: Use mmu_notifier.release() instead of per-vma refcounting
Date: Mon, 8 Jul 2019 07:57:07 -0700 [thread overview]
Message-ID: <20190708145707.GC20433@linux.intel.com> (raw)
In-Reply-To: <20190620210324.GF15383@linux.intel.com>
On Fri, Jun 21, 2019 at 12:03:36AM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 19, 2019 at 03:23:50PM -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).
>
> Why having sgx_vma_close() prevents that? I do not understand the
> problem statement.
vmas that define .vm_close() cannot be merged.
/*
* If the vma has a ->close operation then the driver probably needs to release
* per-vma resources, so we don't attempt to merge those.
*/
static inline int is_mergeable_vma(struct vm_area_struct *vma,
struct file *file, unsigned long vm_flags,
struct vm_userfaultfd_ctx vm_userfaultfd_ctx)
{
...
if (vma->vm_ops && vma->vm_ops->close)
return 0;
if (!is_mergeable_vm_userfaultfd_ctx(vma, vm_userfaultfd_ctx))
return 0;
return 1;
}
>
> > 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.
>
> Right, there is the potential collision with my changes:
>
> 1. Your patch: enclave life-cycle equals life-cycle of all processes
> that are associated with the enclave.
> 2. My (yet be sent) patch: enclave life-cycle equals the life cycle.
>
> I won't rush with my patch. I rather merge neither at this point and
> you can review mine after you come back from your vacation.
>
> > 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.
>
> This paragraph speaks only about "well behaving" software.
Malicious software isn't all that interesting as there are far easier ways
to waste system resources. That being said, the encl_mm allocation can
use GFP_KERNEL_ACCOUNT.
> > 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().
>
> Is it really impossible to send a separate SRCU patch?
I can split out the SRCU as a precursor. It'll likely take me a few days
to get it sent.
> I fully agree with the SRCU whereas rest of this patch is still
> under debate.
>
> If you could do that, I can merge it in no time. It is a small
> step into better direction.
>
> > Cc: Dave Hansen <dave.hansen@intel.com>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>
> Needs to be rebased because the master missing your earlier bug fix.
...
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index 9566eb72d417..c6436bbd4a68 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -132,103 +132,125 @@ static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> > return entry;
> > }
> >
> > -struct sgx_encl_mm *sgx_encl_mm_add(struct sgx_encl *encl,
> > - struct mm_struct *mm)
> > +static void sgx_encl_mm_release_wq(struct work_struct *work)
> > +{
> > + struct sgx_encl_mm *encl_mm =
> > + container_of(work, struct sgx_encl_mm, release_work);
> > +
> > + sgx_encl_mm_release(encl_mm);
> > +}
> > +
> > +/*
> > + * Being a call_srcu() callback, this needs to be short, and sgx_encl_release()
> > + * is anything but short. Do the final freeing in yet another async callback.
> > + */
> > +static void sgx_encl_mm_release_delayed(struct rcu_head *rcu)
>
> Would rename this either as *_tail() or *_deferred().
Deferred works for me.
> > +{
> > + struct sgx_encl_mm *encl_mm =
> > + container_of(rcu, struct sgx_encl_mm, rcu);
> > +
> > + INIT_WORK(&encl_mm->release_work, sgx_encl_mm_release_wq);
> > + schedule_work(&encl_mm->release_work);
> > +}
> > +
...
> > @@ -118,11 +123,13 @@ void sgx_encl_destroy(struct sgx_encl *encl);
> > void sgx_encl_release(struct kref *ref);
> > pgoff_t sgx_encl_get_index(struct sgx_encl *encl, struct sgx_encl_page *page);
> > struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, pgoff_t index);
> > -struct sgx_encl_mm *sgx_encl_next_mm(struct sgx_encl *encl,
> > - struct sgx_encl_mm *encl_mm, int *iter);
> > -struct sgx_encl_mm *sgx_encl_mm_add(struct sgx_encl *encl,
> > - struct mm_struct *mm);
> > -void sgx_encl_mm_release(struct kref *ref);
> > +int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
> > +static inline void sgx_encl_mm_release(struct sgx_encl_mm *encl_mm)
> > +{
> > + kref_put(&encl_mm->encl->refcount, sgx_encl_release);
> > +
> > + kfree(encl_mm);
> > +}
>
> Please just open code this to the two call sites. Makes the code hard to
> follow.
Heh, I waffled between a helper and open coding. I chose poorly :-)
> Right now I did not find anything else questionable from the code
> changes. Repeating myself but if it is by any means possible before
> going away, can you construct a pure SRCU patch.
>
> I could then reconstruct my changes on top off that, which would
> make evalution of both heck a lot easier.
>
> /Jarkko
next prev parent reply other threads:[~2019-07-08 14:57 UTC|newest]
Thread overview: 156+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-19 22:23 [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM Sean Christopherson
2019-06-19 22:23 ` [RFC PATCH v4 01/12] x86/sgx: Use mmu_notifier.release() instead of per-vma refcounting Sean Christopherson
2019-06-20 21:03 ` Jarkko Sakkinen
2019-07-08 14:57 ` Sean Christopherson [this message]
2019-07-09 16:18 ` Jarkko Sakkinen
2019-06-19 22:23 ` [RFC PATCH v4 02/12] x86/sgx: Do not naturally align MAP_FIXED address Sean Christopherson
2019-06-20 21:09 ` Jarkko Sakkinen
2019-06-20 22:09 ` Jarkko Sakkinen
2019-06-19 22:23 ` [RFC PATCH v4 03/12] selftests: x86/sgx: Mark the enclave loader as not needing an exec stack Sean Christopherson
2019-06-20 21:17 ` Jarkko Sakkinen
2019-06-19 22:23 ` [RFC PATCH v4 04/12] x86/sgx: Require userspace to define enclave pages' protection bits Sean Christopherson
2019-06-21 1:07 ` Jarkko Sakkinen
2019-06-21 1:16 ` Jarkko Sakkinen
2019-06-21 16:42 ` Xing, Cedric
2019-07-08 16:34 ` Sean Christopherson
2019-07-08 17:29 ` Xing, Cedric
2019-07-01 18:00 ` Andy Lutomirski
2019-07-01 19:22 ` Xing, Cedric
2019-06-19 22:23 ` [RFC PATCH v4 05/12] x86/sgx: Enforce noexec filesystem restriction for enclaves Sean Christopherson
2019-06-21 1:26 ` Jarkko Sakkinen
2019-07-07 19:03 ` Sean Christopherson
2019-06-19 22:23 ` [RFC PATCH v4 06/12] mm: Introduce vm_ops->may_mprotect() Sean Christopherson
2019-06-21 1:35 ` Jarkko Sakkinen
2019-06-19 22:23 ` [RFC PATCH v4 07/12] LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX Sean Christopherson
2019-06-21 2:28 ` Jarkko Sakkinen
2019-06-21 16:54 ` Xing, Cedric
2019-06-25 20:48 ` Stephen Smalley
2019-06-27 20:29 ` Xing, Cedric
2019-07-07 18:01 ` Sean Christopherson
2019-06-19 22:23 ` [RFC PATCH v4 08/12] security/selinux: Require SGX_MAPWX to map enclave page WX Sean Christopherson
2019-06-21 17:09 ` Xing, Cedric
2019-06-25 21:05 ` Stephen Smalley
2019-06-27 20:26 ` Xing, Cedric
2019-06-25 20:19 ` Stephen Smalley
2019-06-26 12:49 ` Dr. Greg
2019-06-19 22:23 ` [RFC PATCH v4 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX Sean Christopherson
2019-06-21 17:05 ` Xing, Cedric
2019-06-25 21:01 ` Stephen Smalley
2019-06-25 21:49 ` Stephen Smalley
2019-06-27 19:38 ` Xing, Cedric
2019-06-19 22:23 ` [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation Sean Christopherson
2019-06-21 21:22 ` Xing, Cedric
2019-06-25 21:09 ` Stephen Smalley
2019-06-27 20:19 ` Xing, Cedric
2019-06-28 16:16 ` Stephen Smalley
2019-06-28 21:20 ` Xing, Cedric
2019-06-29 1:15 ` Stephen Smalley
2019-07-01 18:14 ` Xing, Cedric
2019-06-29 23:41 ` Andy Lutomirski
2019-07-01 17:46 ` Xing, Cedric
2019-07-01 17:53 ` Andy Lutomirski
2019-07-01 18:54 ` Xing, Cedric
2019-07-01 19:03 ` Xing, Cedric
2019-07-01 19:32 ` Andy Lutomirski
2019-07-01 20:03 ` Xing, Cedric
2019-07-07 18:46 ` Sean Christopherson
2019-06-25 20:34 ` Stephen Smalley
2019-06-19 22:24 ` [RFC PATCH v4 11/12] security/apparmor: " Sean Christopherson
2019-06-19 22:24 ` [RFC PATCH v4 12/12] LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG Sean Christopherson
2019-06-21 17:18 ` Xing, Cedric
2019-07-08 14:34 ` Sean Christopherson
2019-06-21 1:32 ` [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM Jarkko Sakkinen
2019-06-27 18:56 ` [RFC PATCH v2 0/3] security/x86/sgx: SGX specific LSM hooks Cedric Xing
2019-07-03 23:16 ` Jarkko Sakkinen
2019-07-03 23:22 ` Jarkko Sakkinen
2019-07-03 23:23 ` Jarkko Sakkinen
2019-07-06 5:04 ` Xing, Cedric
2019-07-08 14:46 ` Jarkko Sakkinen
2019-07-07 23:41 ` [RFC PATCH v3 0/4] " Cedric Xing
2019-07-08 15:55 ` Sean Christopherson
2019-07-08 17:49 ` Xing, Cedric
2019-07-08 18:49 ` Sean Christopherson
2019-07-08 22:26 ` Xing, Cedric
2019-07-07 23:41 ` [RFC PATCH v3 1/4] x86/sgx: Add " Cedric Xing
2019-07-07 23:41 ` [RFC PATCH v3 2/4] x86/64: Call LSM hooks from SGX subsystem/module Cedric Xing
2019-07-09 1:03 ` Sean Christopherson
2019-07-07 23:41 ` [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module Cedric Xing
2019-07-08 16:26 ` Casey Schaufler
2019-07-08 17:16 ` Xing, Cedric
2019-07-08 23:53 ` Casey Schaufler
2019-07-09 22:13 ` Xing, Cedric
2019-07-10 0:10 ` Casey Schaufler
2019-07-10 0:55 ` Xing, Cedric
2019-07-10 21:14 ` Casey Schaufler
2019-07-11 13:51 ` Stephen Smalley
2019-07-11 15:12 ` Sean Christopherson
2019-07-11 16:11 ` Stephen Smalley
2019-07-11 16:25 ` Sean Christopherson
2019-07-11 16:32 ` Stephen Smalley
2019-07-11 23:41 ` Xing, Cedric
2019-07-07 23:41 ` [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux Cedric Xing
2019-07-09 1:33 ` Sean Christopherson
2019-07-09 21:26 ` Xing, Cedric
2019-07-10 15:49 ` Sean Christopherson
2019-07-10 16:08 ` Jethro Beekman
2019-07-10 18:16 ` Xing, Cedric
2019-07-10 17:54 ` Xing, Cedric
2019-06-27 18:56 ` [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks Cedric Xing
2019-06-27 22:06 ` Casey Schaufler
2019-06-27 22:52 ` Xing, Cedric
2019-06-27 23:37 ` Casey Schaufler
2019-06-28 0:47 ` Xing, Cedric
2019-06-28 17:22 ` Casey Schaufler
2019-06-28 22:29 ` Xing, Cedric
2019-06-29 1:37 ` Stephen Smalley
2019-06-29 21:35 ` Casey Schaufler
2019-07-01 17:57 ` Xing, Cedric
2019-07-01 19:53 ` Casey Schaufler
2019-07-01 21:45 ` Xing, Cedric
2019-07-01 23:11 ` Casey Schaufler
2019-07-02 7:42 ` Xing, Cedric
2019-07-02 15:44 ` Casey Schaufler
2019-07-03 9:46 ` Dr. Greg
2019-07-03 15:32 ` Casey Schaufler
2019-07-07 13:30 ` Dr. Greg
2019-07-09 0:02 ` Casey Schaufler
2019-07-09 1:52 ` Sean Christopherson
2019-07-09 21:16 ` Xing, Cedric
2019-07-11 10:22 ` Dr. Greg
2019-07-15 22:23 ` Andy Lutomirski
2019-06-28 16:37 ` Stephen Smalley
2019-06-28 21:53 ` Xing, Cedric
2019-06-29 1:22 ` Stephen Smalley
2019-07-01 18:02 ` Xing, Cedric
2019-06-29 23:46 ` Andy Lutomirski
2019-07-01 17:11 ` Xing, Cedric
2019-07-01 17:58 ` Andy Lutomirski
2019-07-01 18:31 ` Xing, Cedric
2019-07-01 19:36 ` Andy Lutomirski
2019-07-01 19:56 ` Xing, Cedric
2019-07-02 2:29 ` Andy Lutomirski
2019-07-02 6:35 ` Xing, Cedric
2019-06-27 18:56 ` [RFC PATCH v2 2/3] x86/sgx: Call LSM hooks from SGX subsystem/module Cedric Xing
2019-06-27 18:56 ` [RFC PATCH v2 3/3] x86/sgx: Implement SGX specific hooks in SELinux Cedric Xing
2019-07-05 16:05 ` [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM Jarkko Sakkinen
2019-07-08 17:29 ` Sean Christopherson
2019-07-08 17:33 ` Xing, Cedric
2019-07-09 16:22 ` Jarkko Sakkinen
2019-07-09 17:09 ` Sean Christopherson
2019-07-09 20:41 ` Xing, Cedric
2019-07-09 22:25 ` Sean Christopherson
2019-07-09 23:11 ` Xing, Cedric
2019-07-10 16:57 ` Sean Christopherson
2019-07-10 20:19 ` Jarkko Sakkinen
2019-07-10 20:31 ` Sean Christopherson
2019-07-11 9:06 ` Jarkko Sakkinen
2019-07-10 22:00 ` Jarkko Sakkinen
2019-07-10 22:16 ` Jarkko Sakkinen
2019-07-10 23:16 ` Xing, Cedric
2019-07-11 9:26 ` Jarkko Sakkinen
2019-07-11 14:32 ` Stephen Smalley
2019-07-11 17:51 ` Jarkko Sakkinen
2019-07-12 0:08 ` Xing, Cedric
2019-07-10 1:28 ` Dr. Greg
2019-07-10 2:04 ` Xing, Cedric
2019-07-10 3:21 ` Jethro Beekman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190708145707.GC20433@linux.intel.com \
--to=sean.j.christopherson@intel.com \
--cc=casey.schaufler@intel.com \
--cc=cedric.xing@intel.com \
--cc=dave.hansen@intel.com \
--cc=greg@enjellic.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=jethro@fortanix.com \
--cc=jmorris@namei.org \
--cc=linux-security-module@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=luto@kernel.org \
--cc=sds@tycho.nsa.gov \
--cc=selinux@vger.kernel.org \
--cc=william.c.roberts@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).