linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Jarkko Sakkinen" <jarkko@kernel.org>,
	"Huang, Kai" <kai.huang@intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>
Cc: "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"Dhanraj, Vijay" <vijay.dhanraj@intel.com>,
	"haitao.huang@linux.intel.com" <haitao.huang@linux.intel.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
Subject: Re: [RFC PATCH v4 2/4] x86/sgx: Implement support for MADV_WILLNEED
Date: Fri, 30 Jun 2023 04:57:56 +0300	[thread overview]
Message-ID: <CTPM273Z6Y6S.36I8EUC0M72B7@suppilovahvero> (raw)
In-Reply-To: <CTPLZCBDSLUU.1CPR6DL9J2GE9@suppilovahvero>

On Fri Jun 30, 2023 at 4:54 AM EEST, Jarkko Sakkinen wrote:
> On Fri Jun 30, 2023 at 2:29 AM EEST, Huang, Kai wrote:
> > On Thu, 2023-06-29 at 07:23 -0700, Sean Christopherson wrote:
> > > On Thu, Jun 29, 2023, Kai Huang wrote:
> > > > On Wed, 2023-06-28 at 07:57 -0700, Sean Christopherson wrote:
> > > > > On Wed, Jun 28, 2023, Kai Huang wrote:
> > > > > > (but requires MAP_FIXED).
> > > > > 
> > > > > No, SGX doesn't require MAP_FIXED.  The requirements of ELRANGE make it extremely
> > > > > unlikely that mmap() will succeed, but it's not a strict requirement. 
> > > > 
> > > > Looks w/o MAP_FIXED, the driver just uses the generic mm->get_unmapped_area() to
> > > > return the address, which doesn't guarantee the right address will be returned
> > > > at all.  Especially when ELRANGE is reserved via mmap(NULL), the
> > > > mmap(/dev/sgx_enclave) will not return the correct address no matter what pgoff
> > > > is used IIUC.
> > > > 
> > > > static unsigned long sgx_get_unmapped_area(struct file *file,
> > > >                                            unsigned long addr,
> > > >                                            unsigned long len,
> > > >                                            unsigned long pgoff,
> > > >                                            unsigned long flags)
> > > > {
> > > >         if ((flags & MAP_TYPE) == MAP_PRIVATE)
> > > >                 return -EINVAL;
> > > > 
> > > >         if (flags & MAP_FIXED)
> > > >                 return addr;
> > > > 
> > > >         return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
> > > > }
> > > > 
> > > > So to me userspace has to use MAP_FIXED to get the correct address.
> > > 
> > > No.  As called out below, @addr is still used as a fairly strong hint:
> > > 
> > > unsigned long
> > > arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
> > > 			  const unsigned long len, const unsigned long pgoff,
> > > 			  const unsigned long flags)
> > > {
> > > 	struct vm_area_struct *vma;
> > > 	struct mm_struct *mm = current->mm;
> > > 	unsigned long addr = addr0;
> > > 	struct vm_unmapped_area_info info;
> > > 
> > > 	/* requested length too big for entire address space */
> > > 	if (len > TASK_SIZE)
> > > 		return -ENOMEM;
> > > 
> > > 	/* No address checking. See comment at mmap_address_hint_valid() */
> > > 	if (flags & MAP_FIXED)
> > > 		return addr;
> > > 
> > > 	/* for MAP_32BIT mappings we force the legacy mmap base */
> > > 	if (!in_32bit_syscall() && (flags & MAP_32BIT))
> > > 		goto bottomup;
> > > 
> > > 	/* requesting a specific address */  <==================================
> > > 	if (addr) {
> > > 		addr &= PAGE_MASK;
> > > 		if (!mmap_address_hint_valid(addr, len))
> > > 			goto get_unmapped_area;
> > > 
> > > 		vma = find_vma(mm, addr);
> > > 		if (!vma || addr + len <= vm_start_gap(vma))
> > > 			return addr;
> > > 	}
> > > 
> > > 	...
> > > }
> > > 
> > > 
> > > In practice, I expect most/all userspace implementations do use MAP_FIXED, but
> > > it's not strictly required.
> > > 
> >
> > Yeah I agree it's a strong hint, but from ABI's perspective, I think even a
> > strong hint isn't good enough.  If there's no guarantee userspace can 100% get
> > the correct enclave address, then userspace will always need to verify the
> > return address and if not do mmap() again.
> >
> > Btw, my reading of above function is if @addr hint doesn't work if the ELRANGE
> > is reserved using mmap(NULL), because below code will always NOT return addr:
> >
> > 		vma = find_vma(mm, addr);	<--- A valid VMA will be found
> > 		if (!vma || addr + len <= vm_start_gap(vma))	<--  This check
> > 								 will be false
> > 			return addr;
> >
> > This is kinda reasonable because ELRANGE via mmap(NULL) doesn't have a file
> > backed, so the mmap(/dev/sgx_enclave) should never return an overlapping address
> > even we pass a addr within ELRANGE.
> >
> > But my true argument is from ABI's perspective, although @addr is a hint, but
> > it's not guaranteed the *exact* addr will be returned  (see man page below):
> >
> > "
> > If addr is not NULL, then the kernel takes it as a hint about where to place the
> > mapping; ...... If another mapping already exists there, the kernel picks a new
> > address that may or may not depend on the hint.
> > "
> >
> > But SGX usrespace needs a *exact* address.  MAP_FIXED is the only ABI can
> > guarantee this.
>
> A practical point of view: I don't see much any other point with Intel
> SDK than provide environment to compile and run attestation shenanigans.
>
> Is there something people *actually* use it in the cloud?
>
> I'm starting to miss the perspective on why waste so much energy on this?
> Feels fuzzy.

Does this map to any of the runtimes that people *actually* use for
their workloads?

BR, Jarkko

  reply	other threads:[~2023-06-30  1:58 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
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 [this message]
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

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=CTPM273Z6Y6S.36I8EUC0M72B7@suppilovahvero \
    --to=jarkko@kernel.org \
    --cc=dave.hansen@linux.intel.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=kai.huang@intel.com \
    --cc=linux-sgx@vger.kernel.org \
    --cc=reinette.chatre@intel.com \
    --cc=seanjc@google.com \
    --cc=vijay.dhanraj@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).