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

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

On Mon, 05 Jun 2023 23:11:59 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Fri, 2023-05-26 at 19:32 -0500, Haitao Huang wrote:
>> Hi Kai, Jarkko and Dave
>>
>> On Thu, 09 Mar 2023 05:31:29 -0600, Huang, Kai <kai.huang@intel.com>  
>> wrote:
>> >
>> > So I am still a little bit confused about where does "SGX driver uses
>> > MAP_ANONYMOUS semantics for fd-based mmap()" come from.
>> >
>> > Anyway, we certainly don't want to break userspace.  However, IIUC,  
>> even
>> > from
>> > now on we change the driver to depend on userspace to pass the correct
>> > pgoff in
>> > mmap(), this won't break userspace, because old userspace which  
>> doesn't
>> > use
>> > fadvice() and pgoff actually doesn't matter.  For new userspace which
>> > uses
>> > fadvice(), it needs to pass the correct pgoff.
>> >
>> > I am not saying we should do this, but it doesn't seem we can break
>> > userspace?
>> >
>>
>> Sorry for delayed update but I thought about this more and likely to
>> propose a new EAUG ioctl for this and for enabling SGX-CET shadow stack
>> pages. But regardless, I'd like to wrap up this discussion to just  
>> clarify
>> this anonymous semantics design in documentation so people won't get
>> confused in future.
>>
>> I think we all agree to keep this semantics so no user space would need
>> specify 'offset' for mmap with enclave fd. And here is my proposed
>> documentation changes.
>>
>> --- a/Documentation/x86/sgx.rst
>> +++ b/Documentation/x86/sgx.rst
>> @@ -100,6 +100,23 @@ pages and establish enclave page permissions.
>>                  sgx_ioc_enclave_init
>>                  sgx_ioc_enclave_provision
>>
>> +Enclave memory mapping
>> +----------------------
>> +
>> +A file descriptor created from opening **/dev/sgx_enclave** represents  
>> an
>> +enclave object. The mmap() syscall with enclave file descriptors does  
>> not
>> +support non-zero value for the 'offset' parameter.
>
> I think we all need to understand better why SGX driver requires  
> anonymous
> semantics mmap() against /dev/sgx_enclave, and as a result of that,  
> requires
> mmap() to pass  0 as pgoff (which looks wasn't even discussed when  
> upstreaming
> the driver).
>
> I'll do some investigation and try to summerize and report back.  Thanks.
>
> [...]
>
>> This is
>> +unlike regular file mapping in that no content offset can be defined  
>> that
>> is
>> +independent from the virtual address it is loaded to.
>> +
>>
>
> Don't quite understand this.  The virtual address of a regular file  
> mapping can
> be linked to file's offest from VMA's pgoff.
>

For file mapping, the offset is the 'content offset' relative to the  
beginning of the file content. The file 'content' is independent from the  
memory it is mapped to.

mmap(..., encl_fd, ...) just creates VMAs as windows/views into the  
enclave memory whose range is already defined by [encl->base,  
encl->base+encl->size) when ECREATE is done. The 'content' of enclave and  
the memory to which the 'content' is mapped are the same. Hence, no  
independent  'content offset' can be defined from user point of view.

 From implementation point of view:

In regular file mapping, vma->vm_pgoff has nothing to do with  
vma->vm_start (or 'addr' passed by mmap). It is used to load bytes at  
pg_offset in the 'content' referenced by vma->vm_file, which is backed up  
by a real file or object that contains the bytes.

In enclave mapping, vma->vm_file is the '/dev/sgx_enclave' device node,  
and it does not refer to any content. It does not make sense to have  
'offset' into '/dev/sgx_enclave'. vma->pg_offset is meaningless for  
enclave mapping.

Thanks
Haitao