Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

["Xing, Cedric" <cedric.xing@intel.com>]

On 8/26/2020 1:15 PM, Sean Christopherson wrote:
> On Wed, Aug 26, 2020 at 12:27:54PM -0700, Xing, Cedric wrote:
>> On 8/17/2020 9:24 PM, Sean Christopherson wrote:
>>> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
>>> output params.  In the new struct, add an opaque "user_data" that can be
>>> used to pass context across the vDSO, and an explicit "exit_reason" to
>>> avoid overloading the return value.
>>> In order to pass additional parameters to the exit handler, the exinfo
>> structure could be embedded in a user-defined structure while the handler
>> could pick it up using "container_of" macro. IMO the original interface was
>> neat and suffcient, and we are over-engineering it.
> 
> container_of/offsetof shenanigans were my original suggestion as well.
> Nathaniel's argument is that using such tricks is less than pleasent in a
> Rust environment.
> 
> https://lkml.kernel.org/r/CAOASepOFh-vOrNZEVDFrDSuHs+9GEzzpXUTG-fZMuyjWAkpRWw@mail.gmail.com
> 
I don't know much about rust but there seem to be offsetof/container_of 
solutions in rust. Here's one: 
https://gist.github.com/resilar/baefbfbf0d733c69d70970d829938875.

After countless discussions along this upstream journey, I thought we 
had agreed that simplicity was the priority. And for simplicity we were 
even willing to give up compliance to x86_64 ABI. Then how do you 
justify treating rust better than other programming languages? This 
looks a hard sell to me.

>>> Moving the params into a struct will also make it less painful to use
>>> dedicated exit reasons, and to support exiting on interrupts in future
>>> patches.
>>>
>> My apology for not following discussion lately. What did you mean by
>> "dedicated exit reasons" and why was it "painful"? According to another
>> thread, I guess we wouldn't have "exiting on interrupts".
> 
> Currently the vDSO returns -EFAULT if the enclave encounters an exception,
> which is kludgy for two reasons:
> 
>   1. EFAULT traditionally means "bad address", whereas an exception could be
>      a #UD in the enclave.
> 
>   2. Returning -EFAULT provides weird semantics as it implies the vDSO call
>      failed, which is not the case.  The vDSO itself was successful, i.e. it
>      did the requested EENTER/ERESUME operation, and should really return '0'.
> 
EFAULT is descriptive enough for me. And more importantly 
detailed/specific info is always available in exinfo.

IMO, the purpose of return code is for the caller/handler to branch on. 
If 2 cases share the same execution path, then there's no need to 
distinguish them. In the case of SGX, most (if not all) exceptions are 
handled in the same way - nested EENTER, then what's point of 
distinguishing them? Please note that detailed info is always available 
in exinfo to cover corner cases.

Moreover, even the vDSO API itself cannot tell between faults at 
EENTER/ERESUME (the vDSO itself failes) vs. faults inside the enclave 
(vDSO succeeded but the enclave fails). I don't think you can be 
accurate without complicating the code significantly.

> The proposed solution for #1 is to define arbitrary return values to
> differentiate between synchronous (EEXIT) exits and asynchronous exits due
> to exceptions.  But that doesn't address #2, and gets especially awkward when
> dealing with the call back return, which is also overloaded to use positive
> return values for ENCLU leafs.
> 
> Passing a "run" struct makes it trivially easy to different the return value
> of the vDSO function from the enclave exit reason, and to avoid collisions
> with the return value from the userspace callback.
> 
When a callback is configured, the vDSO NEVER returns to caller directly 
- i.e. all return codes must be from the callback but NOT the vDSO. I 
don't see any collisions here.