Re: [RFC PATCH v4 2/4] x86/sgx: Implement support for MADV_WILLNEED

["Huang, Kai" <kai.huang@intel.com>]

On Tue, 2023-06-27 at 07:50 -0700, Sean Christopherson wrote:
> On Tue, Jun 27, 2023, Kai Huang wrote:
> > > > > 
> > > > > 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?
> 
> To be clear, it's not my perspective, it's simply what the kernel actually does.

Sure.  I was just trying to hear your thoughts :)

> 
> > Is it "SGX driver _requires_ pgoff to be 0 for non-ELRANGE-reserve mmap()", or
> > "SGX driver _ignores_ pgoff"?
> 
> Unless there are checks hiding somewhere, it's the latter.  You might be able to
> get away with changing the kernel to require pgoff to be '0', i.e. if literally
> all users pass in '0', but proving that all users pass '0' is extremely difficult.
> And I don't see any value in requiring pgoff to be '0' for "legacy" users.

I certainly hate to enforce kernel to "require" 0 pgoff from userspace.  I want
to get rid of it.

I believe we both agree "SGX driver _ignores_ pgoff" is the current ABI.

[...]

> 
> I think you're fixated too much on precisely defining the concept of ABI.  
> 

Probably :)

> The
> question that you really want to ask is "could change XYZ break userspace?"

Agreed.

But since "encl->has_mismatched_offsets" is sort of new ABI, I think we need to
be careful otherwise in the future we may hit this kinda nasty issue again.

Here's my thoughts:

(Again, let's forget about mmap(/dev/sgx_enclave) to reserve ELRANGE for now.)

1) Current ABI is SGX driver _ignores_ pgoff for mmap(/dev/sgx_enclave)s (but
requires MAP_FIXED).

2) Therefore, "passing the correct pgoff" in new userspace app doesn't break the
current ABI.  If the new app chooses to pass the correct pgoff, it will work.

3) With additional support of sgx_fadvice(WILLNEED) within the driver, the new
app can use madvice(WILLNEED) if it passes the correct pgoff when mmap().  If
wrong pgoff is passed, then madvice(WILLNEED) will work unexpectedly and could
result in enclave being killed.  It's userspace app's responsibility to make
sure the correctness, not the driver's.

4) Old SGX apps which don't use file-based ABIs and still pass 0 pgoff should
continue to work.  No break of old apps either.

5) We encourage new apps to always pass the correct pgoff instead of passing 0.

6) If needed, we can modify sgx_mmap() to relax the needing to use MAP_FIXED,
but return the enclave's address "based on pgoff from userspace".  This
effectively provides additional mmap() ABI for userspace while not breaking the
existing MAP_FIXED ABI.

In this way, we don't need additional manipulation/fixing up of VMA's pgoff in
the driver.  There's no change to existing ABI either.

[...]


> Because SGX has users in the wild that don't set pgoff correctly.  Changing the
> kernel to require an accurate pgoff would break those users.

Only require an accurate pgoff if userspace wants to use file-based ABIs for the
relevant VMAs.  The old apps which pass 0 pgoff should just continue to work.