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

["Haitao Huang" <haitao.huang@xxxxxxxxxxxxxxx>]

On Tue, 21 Feb 2023 16:10:02 -0600, jarkko@xxxxxxxxxx <jarkko@xxxxxxxxxx>  
wrote:

> On Fri, Feb 17, 2023 at 05:03:05PM -0600, Haitao Huang wrote:
> > Hi Jarkko
> >
> > On Fri, 17 Feb 2023 16:32:12 -0600, jarkko@xxxxxxxxxx  
> > <jarkko@xxxxxxxxxx>
> > wrote:
> >
> > > On Wed, Feb 15, 2023 at 09:42:30AM -0600, Haitao Huang wrote:
> > > > On Wed, 15 Feb 2023 02:51:23 -0600, Huang, Kai <kai.huang@xxxxxxxxx>
> > > > 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.
> > > >
> > > > I still think returning error for cases mmap(..., enclave_fd) if
> > > > enclave is
> > > > not created would be less intrusive change.
> > >
> > > Is this something you care in SGX SDK?
> > >
> > > It is not categorically forbidden so that's why I'm asking.
> >
> > SDK does not care as we would never do this and don't think anyone is  
> > doing
> > that either. Suggesting returning error to cover all cases so user space
> > would not accidentally cause incorrect vm_pgoff set.
>
> vm_pgoff != 0 should result -EINVAL.

Do you mean 'offset' passed in from mmap syscall and converted to pgoff in  
kernel? I can see it only passed to driver in get_unmapped_area. I can add  
this enforcement there so it is consistent to the MAP_ANONYMOUS spec if  
that's what you suggest.

 vma->vm_pgoff is the offset in pages of the vma->vm_start relative to  
'file'.
 It can not be zero for all VMAs of the enclave.

Kernel sets vma->vm_pgoff == vma->vm_start>>PAGE_SHIFT for private anon  
VMAs, and ignore it (zero) for shared anon mapping.

For us, we propose to set it to PFN_DOWN(vma->vm_start - encl->base) upon  
mmap.
The concern Kai raised about encl->base not available in the window  
between mmap and ECREATE can be addressed by disallowing mmap before  
ECREATE is done. It does not make much sense anyway to mmap(enclave_fd)  
without a valid enclave range associated with enclave_fd.

Does that sound reasonable or I still miss some point here?

Thanks
Haitao