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

[Sean Christopherson <seanjc@google.com>]

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.

> > > 2) Therefore,�"passing the correct pgoff" in new userspace app doesn't break the
> > > current ABI.  If the new app chooses to pass the correct pgoff, it will work.
> > 
> > Yep.
> > 
> > > 3) With additional support of sgx_fadvice(WILLNEED) within the driver, the new
> > > app can use madvice(WILLNEED) if it passes the correct pgoff when mmap().  If
> > > wrong pgoff is passed, then madvice(WILLNEED) will work unexpectedly and could
> > > result in enclave being killed.  It's userspace app's responsibility to make
> > > sure the correctness, not the driver's.
> > 
> > Hmm, yeah, it's probably ok to lean on documentation if userspace screws up.  I
> > generally prefer explicit errors over "undefined behavior", but I don't have a
> > super strong opinion, especially since this isn't my area these days :-) 
> 
> My argument is for normal file operations, if you use madvice(WILLNEED) but
> didn't mmap() with the correct pgoff, you will end up with kernel acting on the
> wrong portion of the file (which may not result in fatal error, but certainly
> not the correct thing from userspace's perspective).
> 
> So kernel doesn't guarantee anything here, but depends on userspace to do things
> correctly.

Fair enough.

> > > 4) Old SGX apps which don't use file-based ABIs and still pass 0 pgoff should
> > > continue to work.  No break of old apps either.
> > 
> > Yep.
> >  
> > > 5) We encourage new apps to always pass the correct pgoff instead of passing 0.
> > > 
> > > 6) If needed, we can modify sgx_mmap() to relax the needing to use MAP_FIXED,
> > > but return the enclave's address "based on pgoff from userspace".
> > 
> > As above, this isn't a relaxation of anything since MAP_FIXED isn't strictly
> > required, it's simply additional functionality.
> > 
> > > This effectively provides additional mmap() ABI for userspace while not
> > > breaking the existing MAP_FIXED ABI.
> > 
> > I don't think you want to do this.  The MAP_FIXED case won't be affected, but the
> > address provided to mmap() is also used as a hint in the !MAP_FIXED case,, i.e.
> > MAP_FIXED just means use *exactly* this address and don't try anything else.  It's
> > unlikely, but not _that_ unlikely, that there are userspace applications that
> > specify the exact address without MAP_FIXED and without the correct pgoff.