Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: linux-kernel@vger.kernel.org, x86@kernel.org, linux-sgx@vger.kernel.org
Cc: akpm@linux-foundation.org, dave.hansen@intel.com,
	sean.j.christopherson@intel.com, nhorman@redhat.com,
	npmccallum@redhat.com, serge.ayoun@intel.com,
	shay.katz-zamir@intel.com, haitao.huang@intel.com,
	andriy.shevchenko@linux.intel.com, tglx@linutronix.de,
	kai.svahn@intel.com, bp@alien8.de, josh@joshtriplett.org,
	luto@kernel.org, kai.huang@intel.com, rientjes@google.com,
	cedric.xing@intel.com, Andy Lutomirski <luto@amacapital.net>,
	Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH v22 16/24] x86/vdso: Add support for exception fixup in vDSO functions
Date: Thu, 3 Oct 2019 02:18:04 +0300
Message-ID: <20191002231804.GA14315@linux.intel.com> (raw)
In-Reply-To: <20190903142655.21943-17-jarkko.sakkinen@linux.intel.com>

On Tue, Sep 03, 2019 at 05:26:47PM +0300, Jarkko Sakkinen wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> The basic concept and implementation is very similar to the kernel's
> exception fixup mechanism.  The key differences are that the kernel
> handler is hardcoded and the fixup entry addresses are relative to
> the overall table as opposed to individual entries.

The commit message is missing description of what the commit does.
Please explain "what" before "why". Now "what" is completely lacking.
This paragraph starts as if there was an invisible paragraph before it.

You should start by explaining briefly about this:

1. A brief description of what vdso2c is.
2. A brief description of what changes you do to vdso2.
3. A brief description of what kernel change you do.
4. A brief description of the flow how the expection gets delivered
   to the user space.

All of this is completely missing.

> Hardcoding the kernel handler avoids the need to figure out how to
> get userspace code to point at a kernel function.  Given that the
> expected usage is to propagate information to userspace, dumping all
> fault information into registers is likely the desired behavior for
> the vast majority of yet-to-be-created functions.  Use registers
> DI, SI and DX to communicate fault information, which follows Linux's
> ABI for register consumption and hopefully avoids conflict with
> hardware features that might leverage the fixup capabilities, e.g.
> register usage for SGX instructions was at least partially designed
> with calling conventions in mind.

No description of what is stored in DI, SI and DX. Also there is two
space bars after *every* sentence. Your text editor is totally broken
somehow. Also DB/BP exception is not described.

> Making fixup addresses relative to the overall table allows the table
> to be stripped from the final vDSO image (it's a kernel construct)
> without complicating the offset logic, e.g. entry-relative addressing
> would also need to account for the table's location relative to the
> image.
> 
> Regarding stripping the table, modify vdso2c to extract the table from
> the raw, a.k.a. unstripped, data and dump it as a standalone byte array
> in the resulting .c file.  The original base of the table, its length
> and a pointer to the byte array are captured in struct vdso_image.
> Alternatively, the table could be dumped directly into the struct,
> but because the number of entries can vary per image, that would
> require either hardcoding a max sized table into the struct definition
> or defining the table as a flexible length array.  The flexible length
> array approach has zero benefits, e.g. the base/size are still needed,
> and prevents reusing the extraction code, while hardcoding the max size
> adds ongoing maintenance just to avoid exporting the explicit size.
> 
> The immediate use case is for Intel Software Guard Extensions (SGX).
> SGX introduces a new CPL3-only "enclave" mode that runs as a sort of
> black box shared object that is hosted by an untrusted "normal" CPl3
> process.
> 
> Entering an enclave can only be done through SGX-specific instructions,
> EENTER and ERESUME, and is a non-trivial process.  Because of the
> complexity of transitioning to/from an enclave, the vast majority of
> enclaves are expected to utilize a library to handle the actual
> transitions.  This is roughly analogous to how e.g. libc implementations
> are used by most applications.
> 
> Another crucial characteristic of SGX enclaves is that they can generate
> exceptions as part of their normal (at least as "normal" as SGX can be)
> operation that need to be handled *in* the enclave and/or are unique
> to SGX.
> 
> And because they are essentially fancy shared objects, a process can
> host any number of enclaves, each of which can execute multiple threads
> simultaneously.
> 
> Putting everything together, userspace enclaves will utilize a library
> that must be prepared to handle any and (almost) all exceptions any time
> at least one thread may be executing in an enclave.  Leveraging signals
> to handle the enclave exceptions is unpleasant, to put it mildly, e.g.
> the SGX library must constantly (un)register its signal handler based
> on whether or not at least one thread is executing in an enclave, and
> filter and forward exceptions that aren't related to its enclaves.  This
> becomes particularly nasty when using multiple levels of libraries that
> register signal handlers, e.g. running an enclave via cgo inside of the
> Go runtime.
> 
> Enabling exception fixup in vDSO allows the kernel to provide a vDSO
> function that wraps the low-level transitions to/from the enclave, i.e.
> the EENTER and ERESUME instructions.  The vDSO function can intercept
> exceptions that would otherwise generate a signal and return the fault
> information directly to its caller, thus avoiding the need to juggle
> signal handlers.
> 
> Note that unlike the kernel's _ASM_EXTABLE_HANDLE implementation, the
> 'C' version of _ASM_VDSO_EXTABLE_HANDLE doesn't use a pre-compiled
> assembly macro.  Duplicating four lines of code is simpler than adding
> the necessary infrastructure to generate pre-compiled assembly and the
> intended benefit of massaging GCC's inlining algorithm is unlikely to
> realized in the vDSO any time soon, if ever.

Rest of the story is a mess with bits of pieces of "what" and "why"
and mixed together. You could probably make the whole like 50% smaller
with a better organization.

I never understood anything of this commit message. Only by looking
at the code change and completely ignoring the commit message I could
understand what the heck is going on. The commit message in the current
for makes me understand *less* the code change.

It would be even better to delete it completely than have it in the
current form. I would suggest that you do that and concentrate writing
steps 1-4 that I described above.

/Jarkko

  reply index

Thread overview: 102+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-03 14:26 [PATCH v22 00/24] Intel SGX foundations Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 01/24] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2019-09-24 15:28   ` Borislav Petkov
2019-09-24 16:11     ` Sean Christopherson
2019-09-24 16:25       ` Borislav Petkov
2019-09-03 14:26 ` [PATCH v22 02/24] x86/cpufeatures: x86/msr: Intel SGX Launch Control " Jarkko Sakkinen
2019-09-24 15:52   ` Borislav Petkov
2019-09-24 20:22     ` Sean Christopherson
2019-09-25  8:51       ` Borislav Petkov
2019-09-25 17:18         ` Sean Christopherson
2019-09-25 18:31           ` Borislav Petkov
2019-09-25 19:08             ` Sean Christopherson
2019-09-27 16:11           ` Jarkko Sakkinen
2019-09-25 14:09     ` Jarkko Sakkinen
2019-09-25 14:10       ` Jarkko Sakkinen
2019-09-25 14:38         ` Jarkko Sakkinen
2019-09-25 15:19       ` Borislav Petkov
2019-09-25 16:49         ` Sean Christopherson
2019-09-25 17:28           ` Borislav Petkov
2019-09-25 18:18             ` Sean Christopherson
2019-09-03 14:26 ` [PATCH v22 03/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2019-09-24 16:04   ` Borislav Petkov
2019-09-25 14:16     ` Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 04/24] x86/cpu/intel: Detect SGX supprt Jarkko Sakkinen
2019-09-24 16:13   ` Borislav Petkov
2019-09-24 17:43     ` Sean Christopherson
2019-09-24 18:21       ` Borislav Petkov
2019-09-25 14:46         ` Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 05/24] x86/sgx: Add ENCLS architectural error codes Jarkko Sakkinen
2019-09-27 10:20   ` Borislav Petkov
2019-09-27 16:08     ` Jarkko Sakkinen
2019-09-27 17:20       ` Sean Christopherson
2019-10-01 20:23         ` Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 06/24] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen
2019-09-27 16:27   ` Borislav Petkov
2019-10-01 19:10     ` Jarkko Sakkinen
2019-10-01 20:39     ` Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 07/24] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2019-10-04  9:45   ` Borislav Petkov
2019-10-04 18:56     ` Jarkko Sakkinen
2019-10-08  4:04     ` Sean Christopherson
2019-10-08  7:18       ` Borislav Petkov
2019-10-08 13:35         ` Sean Christopherson
2019-10-08 14:56           ` Borislav Petkov
2019-09-03 14:26 ` [PATCH v22 08/24] x86/sgx: Enumerate and track EPC sections Jarkko Sakkinen
2019-10-05  9:26   ` Borislav Petkov
2019-10-07 11:58     ` Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 09/24] x86/sgx: Add functions to allocate and free EPC pages Jarkko Sakkinen
2019-10-05 16:44   ` Borislav Petkov
2019-10-07 14:50     ` Sean Christopherson
2019-10-08  9:09       ` Borislav Petkov
2019-10-08 13:31         ` Sean Christopherson
2019-10-07 17:55     ` Jarkko Sakkinen
2019-10-07 18:09       ` Borislav Petkov
2019-09-03 14:26 ` [PATCH v22 10/24] x86/sgx: Add sgx_einit() for wrapping ENCLS[EINIT] Jarkko Sakkinen
2019-10-08 17:30   ` Borislav Petkov
2019-10-08 17:45     ` Sean Christopherson
2019-10-08 17:46       ` Sean Christopherson
2019-10-08 17:53         ` Borislav Petkov
2019-09-03 14:26 ` [PATCH v22 11/24] mm: Introduce vm_ops->may_mprotect() Jarkko Sakkinen
2019-10-08 17:41   ` Borislav Petkov
2019-09-03 14:26 ` [PATCH v22 12/24] x86/sgx: Linux Enclave Driver Jarkko Sakkinen
2019-10-08 17:59   ` Borislav Petkov
2019-10-08 18:17     ` Sean Christopherson
2019-10-08 19:19       ` Borislav Petkov
2019-09-03 14:26 ` [PATCH v22 13/24] x86/sgx: Add provisioning Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 14/24] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 15/24] x86/sgx: ptrace() support for the SGX driver Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 16/24] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2019-10-02 23:18   ` Jarkko Sakkinen [this message]
2019-10-02 23:45     ` Jarkko Sakkinen
2019-10-04  0:03     ` Sean Christopherson
2019-10-04 18:49       ` Jarkko Sakkinen
2019-10-04  0:15     ` Sean Christopherson
2019-10-04 18:52       ` Jarkko Sakkinen
2019-10-05 15:54         ` Sean Christopherson
2019-10-07  7:57           ` Jarkko Sakkinen
2019-10-07  8:10             ` Jarkko Sakkinen
2019-10-07 12:04               ` Jarkko Sakkinen
2019-10-08  4:54                 ` Sean Christopherson
2019-10-05 18:39         ` Sean Christopherson
2019-10-07  8:01           ` Jarkko Sakkinen
2019-10-06 23:38         ` Jarkko Sakkinen
2019-10-06 23:40           ` Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 17/24] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 18/24] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 19/24] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 20/24] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 21/24] selftests/x86: Recurse into subdirectories Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 22/24] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2019-09-03 14:26 ` [PATCH v22 23/24] docs: x86/sgx: Document microarchitecture Jarkko Sakkinen
2019-09-27 18:15   ` Randy Dunlap
2019-09-03 14:26 ` [PATCH v22 24/24] docs: x86/sgx: Document kernel internals Jarkko Sakkinen
2019-09-27 17:07   ` Randy Dunlap
2019-10-01 19:34     ` Jarkko Sakkinen
2019-09-13 20:38 ` [PATCH v22 00/24] Intel SGX foundations Dave Hansen
2019-09-14 13:41   ` Jarkko Sakkinen
2019-09-14 15:32     ` Dave Hansen
2019-09-16  5:23       ` Jarkko Sakkinen
2019-09-24 17:20         ` Andy Lutomirski
2019-09-25 14:32           ` Jarkko Sakkinen
2019-10-02 23:42             ` Jarkko Sakkinen

Reply instructions:

You may reply publically 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=20191002231804.GA14315@linux.intel.com \
    --to=jarkko.sakkinen@linux.intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=haitao.huang@intel.com \
    --cc=josh@joshtriplett.org \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=luto@kernel.org \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=rientjes@google.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=serge.ayoun@intel.com \
    --cc=shay.katz-zamir@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /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

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org
	public-inbox-index linux-sgx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git