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

["Huang, Kai" <kai.huang@intel.com>]

On Thu, 2023-06-29 at 07:23 -0700, Sean Christopherson wrote:
> On Thu, Jun 29, 2023, Kai Huang wrote:
> > On Wed, 2023-06-28 at 07:57 -0700, Sean Christopherson wrote:
> > > On Wed, Jun 28, 2023, Kai Huang wrote:
> > > > (but requires MAP_FIXED).
> > > 
> > > No, SGX doesn't require MAP_FIXED.  The requirements of ELRANGE make it extremely
> > > unlikely that mmap() will succeed, but it's not a strict requirement. 
> > 
> > Looks w/o MAP_FIXED, the driver just uses the generic mm->get_unmapped_area() to
> > return the address, which doesn't guarantee the right address will be returned
> > at all.  Especially when ELRANGE is reserved via mmap(NULL), the
> > mmap(/dev/sgx_enclave) will not return the correct address no matter what pgoff
> > is used IIUC.
> > 
> > static unsigned long sgx_get_unmapped_area(struct file *file,
> >                                            unsigned long addr,
> >                                            unsigned long len,
> >                                            unsigned long pgoff,
> >                                            unsigned long flags)
> > {
> >         if ((flags & MAP_TYPE) == MAP_PRIVATE)
> >                 return -EINVAL;
> > 
> >         if (flags & MAP_FIXED)
> >                 return addr;
> > 
> >         return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
> > }
> > 
> > So to me userspace has to use MAP_FIXED to get the correct address.
> 
> No.  As called out below, @addr is still used as a fairly strong hint:
> 
> unsigned long
> arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
> 			  const unsigned long len, const unsigned long pgoff,
> 			  const unsigned long flags)
> {
> 	struct vm_area_struct *vma;
> 	struct mm_struct *mm = current->mm;
> 	unsigned long addr = addr0;
> 	struct vm_unmapped_area_info info;
> 
> 	/* requested length too big for entire address space */
> 	if (len > TASK_SIZE)
> 		return -ENOMEM;
> 
> 	/* No address checking. See comment at mmap_address_hint_valid() */
> 	if (flags & MAP_FIXED)
> 		return addr;
> 
> 	/* for MAP_32BIT mappings we force the legacy mmap base */
> 	if (!in_32bit_syscall() && (flags & MAP_32BIT))
> 		goto bottomup;
> 
> 	/* requesting a specific address */  <==================================
> 	if (addr) {
> 		addr &= PAGE_MASK;
> 		if (!mmap_address_hint_valid(addr, len))
> 			goto get_unmapped_area;
> 
> 		vma = find_vma(mm, addr);
> 		if (!vma || addr + len <= vm_start_gap(vma))
> 			return addr;
> 	}
> 
> 	...
> }
> 
> 
> In practice, I expect most/all userspace implementations do use MAP_FIXED, but
> it's not strictly required.
> 

Yeah I agree it's a strong hint, but from ABI's perspective, I think even a
strong hint isn't good enough.  If there's no guarantee userspace can 100% get
the correct enclave address, then userspace will always need to verify the
return address and if not do mmap() again.

Btw, my reading of above function is if @addr hint doesn't work if the ELRANGE
is reserved using mmap(NULL), because below code will always NOT return addr:

		vma = find_vma(mm, addr);	<--- A valid VMA will be found
		if (!vma || addr + len <= vm_start_gap(vma))	<--  This check
								 will be false
			return addr;

This is kinda reasonable because ELRANGE via mmap(NULL) doesn't have a file
backed, so the mmap(/dev/sgx_enclave) should never return an overlapping address
even we pass a addr within ELRANGE.

But my true argument is from ABI's perspective, although @addr is a hint, but
it's not guaranteed the *exact* addr will be returned  (see man page below):

"
If addr is not NULL, then the kernel takes it as a hint about where to place the
mapping; ...... If another mapping already exists there, the kernel picks a new
address that may or may not depend on the hint.
"

But SGX usrespace needs a *exact* address.  MAP_FIXED is the only ABI can
guarantee this.
> > >