linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Huang, Kai" <kai.huang@intel.com>
To: "Christopherson,, Sean" <seanjc@google.com>
Cc: "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"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: Tue, 27 Jun 2023 11:43:04 +0000	[thread overview]
Message-ID: <7e78ec8fd22ad9f8b828fa00b9811bbfcf855b2c.camel@intel.com> (raw)
In-Reply-To: <ZJoRHU1CHxIuDH29@google.com>

> > > 
> > > You can't build an ABI on assumptions.  E.g. even if userspace *intends* to behave,
> > > it wouldn't take much to compute the wrong offset (math is hard).
> > 
> > But I don't think we have an well established ABI now?  Nothing is documented.
> 
> Heh, just because the ABI isn't formally documented doesn't mean it doesn't exist.
> In fact, the most problematic ABIs are the ones that aren't documented.
> 
> > 

Sure.  But just want to make sure, what is the current SGX driver ABI (around
mmap()) from your perspective?

Is it "SGX driver _requires_ pgoff to be 0 for non-ELRANGE-reserve mmap()", or
"SGX driver _ignores_ pgoff"?

See below ...

[...]

> > > 	if (encl->has_mismatched_offsets)  <======
> > > 		goto unlock;
> > 
> > Sorry I am a little bit slow, how do you set "has_mismatched_offsests" to true?
> 
> During sgx_mmap().  Though there's a wrinkle I initially missed: if the enclave
> hasn't gone through ECREATE, encl->base is garbage.  So either sgx_mmap() needs
> to assume the !CREATED is creating a mismatched offset, or sgx_encl_create() needs
> to iterate over and check all VMAs.
> 
> Since there are advantages to usuing mmap(NULL) to find ELRANGE, IMO your best
> option is to do the below.  And then that mostly answers the question about
> using mmap(/dev/sgx_enclave) to reserve ELRANGE, i.e. if userspace wants to use
> fallocate(), then it effectively *must not* use mmap(/dev/sgx_enclave).
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 262f5fb18d74..63fb41da35aa 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -94,6 +94,11 @@ static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
>         if (ret)
>                 return ret;
>  
> +       if (!test_bit(SGX_ENCL_CREATED, &encl->flags) ||
> +           vma->vm_pgoff != PFN_DOWN(vma->vm_start - encl->base))
> +               encl->has_mismatched_offsests = true;
> +
> +
>         vma->vm_ops = &sgx_vm_ops;
>         vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO);
>         vma->vm_private_data = encl;
> 

It appears we don't need to mark "has_mismatched_offsets" as true if userspace
uses mmap(/dev/sgx_enclave) to reserve ELRANGE?  Enclave's base isn't
established yet, so theoretically, from enclave's perspective, we cannot say it
has a mismatched offset.

However we may want to return -EINVAL if userspace passes a non-zero pgoff in
the mmap(/dev/sgx_enclave) to reserve ELRANGE.

Anyway reserving ELRANGE isn't the big point.  The SGX driver ABI is.  Let's
forget about reserving ELRANGE in below context, i.e., all mmap() below means
non-ELRANGE-reserve mmap(/dev/sgx_enclave) :)

I think yes marking enclave as "has_mismatched_offsets" works if userspace wants
to use file-based ABIs (fadvice(), etc) for enclave, but this effectively means
Haitao needs to modify _ALL_ existing mmap()s to pass the correct pgoff in order
to use fadvice() for EAUG.

Therefore, it seems more like we are changing ABI (or having a new ABI) to:

Userspace must pass the correct pgoff to all mmaps() in order to use file-based
ABIs for SGX.

( Because I think other drivers/subsystems doesn't have such limitation.  For
instance, for a normal file, userspace can have two mmap()s mapping to the same
offset of the same file but with different address, and fadvice() should work
for both mappings.

A VMA-based "has_mismatched_offset" seems more reasonable for SGX but as I
mentioned previously it may not easy to do from SGX driver. )

So I am not sure whether this belongs to "breaking existing ABI"?

If the current ABI is we _require_ pgoff being zero, then the "encl-
>has_mismatched_offsets" seems is breaking this ABI.

If the current ABI is we _ignore_ pgoff, then "encl->has_mismatched_offsets"
isn't breaking existing ABI.  But in this case why do we need this at all?  SGX
driver just follows the existing file-based ABIs:

If userspace wants to use file-based ABIs for some VMA, it needs to pass the
correct pgoff.  If not, file-based ABIs won't work for that VMA, but your
enclave may still work if you don't use file-based ABIs.

Sorry for being noisy, but does above make sense?



  reply	other threads:[~2023-06-27 11:43 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 [this message]
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

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=7e78ec8fd22ad9f8b828fa00b9811bbfcf855b2c.camel@intel.com \
    --to=kai.huang@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=jarkko@kernel.org \
    --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).