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

jarkko
2023-02-17T22:32:21Z

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

BR, Jarkko