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

From: Sean Christopherson <seanjc@google.com>
To: Kai Huang <kai.huang@intel.com>
Cc: "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	Reinette Chatre <reinette.chatre@intel.com>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	Vijay Dhanraj <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: Wed, 28 Jun 2023 07:57:27 -0700	[thread overview]
Message-ID: <ZJxKV1/2M+OeIXAN@google.com> (raw)
In-Reply-To: <1a980d3ce2caec1cf44bf97d52fa08fbe72e741f.camel@intel.com>

On Wed, Jun 28, 2023, Kai Huang wrote:
> On Tue, 2023-06-27 at 07:50 -0700, Sean Christopherson wrote:
> > 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

Yes.

> (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. 

> 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.

Yep.

> 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.

Hmm, yeah, it's probably ok to lean on documentation if userspace screws up.  I
generally prefer explicit errors over "undefined behavior", but I don't have a
super strong opinion, especially since this isn't my area these days :-) 

> 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.

Yep.

> 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".

As above, this isn't a relaxation of anything since MAP_FIXED isn't strictly
required, it's simply additional functionality.

> This effectively provides additional mmap() ABI for userspace while not
> breaking the existing MAP_FIXED ABI.

I don't think you want to do this.  The MAP_FIXED case won't be affected, but the
address provided to mmap() is also used as a hint in the !MAP_FIXED case,, i.e.
MAP_FIXED just means use *exactly* this address and don't try anything else.  It's
unlikely, but not _that_ unlikely, that there are userspace applications that
specify the exact address without MAP_FIXED and without the correct pgoff.

If there were more to gain by steering mmap() based solely on pgoff, then it might
be worth trying, but this doesn't seem to provide much value to userspace since
the virtual address of any given enclave mapping is mission critical, e.g. it seems
unlikely that userspace wouldn't already know the virtual address it needs.