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

["Haitao Huang" <haitao.huang@linux.intel.com>]

Hi Kai

On Thu, 16 Feb 2023 01:53:47 -0600, Huang, Kai <kai.huang@intel.com> wrote:

> On Wed, 2023-02-15 at 09:42 -0600, Haitao Huang wrote:
>> On Wed, 15 Feb 2023 02:51:23 -0600, Huang, Kai <kai.huang@intel.com>  
>> wrote:
>>
>> > > > > >
>> > > >
>> > > > Since sgx_mmap() can happen before enclave is created,  
>> calculating the
>> > > > vm_pgoff
>> > > > from enclave_base is conceptually wrong.  Even if you really want  
>> to
>> > > do
>> > > > it, it
>> > > > should be:
>> > > >
>> > > > 	if (enclave_has_initialized())
>> > > > 		vma->vm_pgoff = ...;
>> > >
>> > > I got your point now. I can add a condition to test the  
>> SGX_ENCL_CREATED
>> > > bit. However, we still have a hole if we must handle the sequence
>> > > mmap(..., enclave_fd) being called before ECREATE ioctl. We can't  
>> leave
>> > > vm_pgoff not set for those cases.
>> > >
>> > > Since no one does that so far, can we explicitly return an error  
>> from
>> > > sgx_mmap when that happens?
>> > > Other suggestions?
>> >
>> > As I replied to patch 4/4, I believe userspace should pass the correct
>> > pgoff in
>> > mmap().  It's wrong to always pass 0 or any random value.
>> >
>> > If userspace follow the mmap() rule, you won't need to manually set
>> > vm_pgoff
>> > here (which is hacky IMHO).  Everything works fine.
>> >
>>
>> SGX driver was following MAP_ANONYMOUS semantics. If we change that,  
>> it'd
>> break current usage/ABI.
>
> Is there any doc/reference mentioning this?
>
No, just from how we use it :-). Jarkko/Sean/Dave may have  
history/insights on this?
But file offset in mmap spec implies there is a "real" file independent of  
the memory it is mapped into, and you can mmap arbitrary offset of that  
file to any memory address. That does not fit well with SGX enclave  
memory: there is no separate physical file other than enclave residing in  
memory and its base is fixed.
Again, my main concern is impact to existing usage.

>>
>> I still think returning error for cases mmap(..., enclave_fd) if enclave
>> is not created would be less intrusive change.
>
> But you need the first mmap() to work before IOCTL(ecreate) to get  
> enclave's
> base address, correct?
>

No, mmap(..., MAP_ANONYMOUS, fd=-1) must be used to reserve address range  
for enclave before calling IOCTL(ecreate). In fact, I realize mmap(...,  
fd=enclave_fd) before ioctl(ecreate) should fail in sgx_encl_may_map  
according to its definition but IIUC it would skip the loop xas_for_each  
when no entries yet available in encl->page_array. This might be a hole to  
enforcement of vma_prot_bits not exceeding the vm_max_prot_bits which is  
recorded first during EADD.
So if my understanding is correct, we just need fix sgx_encl_may_map to  
return -EACCES when mmap is called before ecreate is done.

Thanks
Haitao