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

jarkko
2023-02-21T22:10:10Z

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@kernel.org <jarkko@kernel.org>
> 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@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.
>
> 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.

BR, Jarkko