From: "Huang, Kai" <firstname.lastname@example.org>
To: "email@example.com" <firstname.lastname@example.org>, "Christopherson,,
Cc: "email@example.com" <firstname.lastname@example.org>,
"Chatre, Reinette" <email@example.com>,
"Dhanraj, Vijay" <firstname.lastname@example.org>,
Subject: Re: [RFC PATCH v4 2/4] x86/sgx: Implement support for MADV_WILLNEED
Date: Fri, 16 Jun 2023 03:49:31 +0000 [thread overview]
Message-ID: <email@example.com> (raw)
On Tue, 2023-06-06 at 04:11 +0000, Huang, Kai wrote:
> On Fri, 2023-05-26 at 19:32 -0500, Haitao Huang wrote:
> > Hi Kai, Jarkko and Dave
> > On Thu, 09 Mar 2023 05:31:29 -0600, Huang, Kai <firstname.lastname@example.org> wrote:
> > >
> > > So I am still a little bit confused about where does "SGX driver uses
> > > MAP_ANONYMOUS semantics for fd-based mmap()" come from.
> > >
> > > Anyway, we certainly don't want to break userspace. However, IIUC, even
> > > from
> > > now on we change the driver to depend on userspace to pass the correct
> > > pgoff in
> > > mmap(), this won't break userspace, because old userspace which doesn't
> > > use
> > > fadvice() and pgoff actually doesn't matter. For new userspace which
> > > uses
> > > fadvice(), it needs to pass the correct pgoff.
> > >
> > > I am not saying we should do this, but it doesn't seem we can break
> > > userspace?
> > >
> > Sorry for delayed update but I thought about this more and likely to
> > propose a new EAUG ioctl for this and for enabling SGX-CET shadow stack
> > pages. But regardless, I'd like to wrap up this discussion to just clarify
> > this anonymous semantics design in documentation so people won't get
> > confused in future.
> > I think we all agree to keep this semantics so no user space would need
> > specify 'offset' for mmap with enclave fd. And here is my proposed
> > documentation changes.
> > --- a/Documentation/x86/sgx.rst
> > +++ b/Documentation/x86/sgx.rst
> > @@ -100,6 +100,23 @@ pages and establish enclave page permissions.
> > sgx_ioc_enclave_init
> > sgx_ioc_enclave_provision
> > +Enclave memory mapping
> > +----------------------
> > +
> > +A file descriptor created from opening **/dev/sgx_enclave** represents an
> > +enclave object. The mmap() syscall with enclave file descriptors does not
> > +support non-zero value for the 'offset' parameter.
> I think we all need to understand better why SGX driver requires anonymous
> semantics mmap() against /dev/sgx_enclave, and as a result of that, requires
> mmap() to pass 0 as pgoff (which looks wasn't even discussed when upstreaming
> the driver).
> I'll do some investigation and try to summerize and report back. Thanks.
If you see this and have time, please help to comment. Thanks.
I've spent plenty of time to look into the discussions around v20/v28/v29 and
roughly v38/v39 to find out why SGX driver requires MAP_ANONYMOUS semantics, and
AFAICT it turns out it was never explicitly discussed. Or perhaps the
"MAP_ANONYMOUS semantics" actually just means "MAP_SHARED | MAP_FIXED + pgoff is
ignored", and everyone believed there was no need to explain what does "SGX
driver uses MAP_ANONYMOUS semantics for mmap()" mean.
The v20 story (that I spent most of my time on) mentioned by Haitao was actually
about how to make SGX and LSM work together but not related to SGX driver mmap()
Also Haitao mentioned "the use of anonymous mapping can be traced back to v29"
but this actually was just about how to use the first mmap() to "reserve the
ELRANGE before ECREATE". It wasn't about to changing mmap(/dev/sgx_enclave)
semantics at all.
Sean actually suggested to explicitly document "how does SGX driver recommend
the user to reserve ELRANGE", but Jarkko didn't think we should do:
which is a pity IMHO, because I believe for anyone, naturally, the first
instinct to reserve ELRANGE is to use mmap(/dev/sgx_enclave) but not
mmap(MAP_ANONYMOUS). If we suggest user to use the latter then there must be
some reason and IMHO such suggestion and reason should be documented.
Also, if I am not missing something, the current driver doesn't prevent using
mmap(/dev/sgx_enclave, PROT_NONE) to reserve ELANGE. So having clear
documentation is helpful for SGX users to choose how to write their apps.
Go back to the "SGX driver uses MAP_ANONYMOUS semantics for mmap()", I believe
this just is "SGX driver requires mmap() after ECREATE/EINIT to use MAP_SHARED |
MAP_FIXED and pgoff is ignored". Or more precisely, pgoff is "not _used_ by SGX
In fact, I think "pgoff is ignored/not used" is technically wrong for enclave.
Pgoff is ignored in case of MAP_SHARED | MAP_ANONYMOUS makes sense, because you
get a new shmem file everytime you do so. But this isn't the case for enclave.
For all mmap()s against the same enclave, pgoff has a valid meaning. SGX driver
doesn't use vma->pgoff thus it's OK to not have valid vma->pgoff but this
confuses the core-MM, because now we can easily end up having multiple VMAs
mapping to different part of enclave, but core-MM believes they all map to the
start of the enclave.
For instance, have we tested all corner cases around VMA splitting/merging, etc?
IMHO we should stop saying SGX driver uses MAP_ANONYMOUS semantics, because the
truth is it just takes advantage of MAP_FIXED and carelessly ignores the pgoff
due to the nature of SGX w/o considering from core-MM's perspective.
And IMHO there are two ways to fix:
1) From now on, we ask SGX apps to use the correct pgoff in their
mmap(/dev/sgx_enclave). This shouldn't impact the existing SGX apps because SGX
driver doesn't use vma->pgoff anyway.
2) For the sake of avoiding having to ask existing SGX apps to change their
mmap()s, we _officially_ say that userspace isn't required to pass a correct
pgoff to mmap() (i.e. passing 0 as did in existing apps), but the kernel should
fix the vma->pgoff internally.
I do prefer option 2) because it has no harm to anyone: 1) No changes to
existing SGX apps; 2) It aligns with the core-MM to so all existing mmap()
operations should work as expected, meaning no surprise; 3) And this patchset
from Haitao to use fadvice() to accelerate EAUG flow just works.
And I believe we should document all those staffs so everyone can understand.
next prev parent reply other threads:[~2023-06-16 3:49 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-28 4:55 [RFC PATCH v4 0/4] x86/sgx: implement support for MADV_WILLNEED Haitao Huang
2023-01-28 4:55 ` [RFC PATCH v4 1/4] x86/sgx: Export sgx_encl_eaug_page Haitao Huang
2023-01-28 4:55 ` [RFC PATCH v4 2/4] x86/sgx: Implement support for MADV_WILLNEED Haitao Huang
2023-01-28 4:55 ` [RFC PATCH v4 3/4] selftests/sgx: add len field for EACCEPT op Haitao Huang
2023-01-28 4:55 ` [RFC PATCH v4 4/4] selftests/sgx: Add test for madvise(..., WILLNEED) Haitao Huang
2023-02-07 23:30 ` Jarkko Sakkinen
2023-02-15 2:38 ` Huang, Kai
2023-02-15 4:42 ` Haitao Huang
2023-02-15 8:46 ` Huang, Kai
2023-02-17 22:29 ` jarkko
2023-02-07 23:29 ` [RFC PATCH v4 3/4] selftests/sgx: add len field for EACCEPT op Jarkko Sakkinen
2023-02-07 23:28 ` [RFC PATCH v4 2/4] x86/sgx: Implement support for MADV_WILLNEED Jarkko Sakkinen
2023-02-14 9:47 ` Huang, Kai
2023-02-14 19:18 ` Haitao Huang
2023-02-14 20:54 ` Huang, Kai
2023-02-14 21:42 ` Haitao Huang
2023-02-14 22:36 ` Huang, Kai
2023-02-15 3:59 ` Haitao Huang
2023-02-15 8:51 ` Huang, Kai
2023-02-15 15:42 ` Haitao Huang
2023-02-16 7:53 ` Huang, Kai
2023-02-16 17:12 ` Haitao Huang
2023-02-17 22:32 ` jarkko
2023-02-17 23:03 ` Haitao Huang
2023-02-21 22:10 ` jarkko
2023-02-22 1:37 ` Haitao Huang
2023-03-07 23:32 ` Huang, Kai
2023-03-09 0:50 ` Haitao Huang
2023-03-09 11:31 ` Huang, Kai
2023-03-14 14:54 ` Haitao Huang
2023-03-19 13:26 ` jarkko
2023-03-20 9:36 ` Huang, Kai
2023-03-20 14:04 ` jarkko
2023-05-27 0:32 ` Haitao Huang
2023-06-06 4:11 ` Huang, Kai
2023-06-07 16:59 ` Haitao Huang
2023-06-16 3:49 ` Huang, Kai [this message]
2023-06-16 22:05 ` Sean Christopherson
2023-06-19 11:17 ` Huang, Kai
2023-06-22 22:01 ` Sean Christopherson
2023-06-22 23:21 ` Huang, Kai
2023-06-26 22:28 ` Sean Christopherson
2023-06-27 11:43 ` Huang, Kai
2023-06-27 14:50 ` Sean Christopherson
2023-06-28 9:37 ` Huang, Kai
2023-06-28 14:57 ` Sean Christopherson
2023-06-29 3:10 ` Huang, Kai
2023-06-29 14:23 ` Sean Christopherson
2023-06-29 23:29 ` Huang, Kai
2023-06-30 0:14 ` Sean Christopherson
2023-06-30 0:56 ` Huang, Kai
2023-06-30 1:54 ` Jarkko Sakkinen
2023-06-30 1:57 ` Jarkko Sakkinen
2023-06-30 4:26 ` Huang, Kai
2023-06-30 9:35 ` Jarkko Sakkinen
2023-03-12 1:25 ` jarkko
2023-03-12 22:25 ` Huang, Kai
2023-02-17 22:07 ` jarkko
2023-02-17 21:50 ` jarkko
2023-02-07 23:26 ` [RFC PATCH v4 1/4] x86/sgx: Export sgx_encl_eaug_page Jarkko Sakkinen
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* 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).