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

Haitao Huang
2023-02-22T01:37:24Z

On Tue, 21 Feb 2023 16:10:02 -0600, jarkko@kernel.org <jarkko@kernel.org>
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@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.
>

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