> > > > > > You can't build an ABI on assumptions. E.g. even if userspace *intends* to behave, > > > it wouldn't take much to compute the wrong offset (math is hard). > > > > But I don't think we have an well established ABI now? Nothing is documented. > > Heh, just because the ABI isn't formally documented doesn't mean it doesn't exist. > In fact, the most problematic ABIs are the ones that aren't documented. > > > Sure. But just want to make sure, what is the current SGX driver ABI (around mmap()) from your perspective? Is it "SGX driver _requires_ pgoff to be 0 for non-ELRANGE-reserve mmap()", or "SGX driver _ignores_ pgoff"? See below ... [...] > > > if (encl->has_mismatched_offsets) <====== > > > goto unlock; > > > > Sorry I am a little bit slow, how do you set "has_mismatched_offsests" to true? > > During sgx_mmap(). Though there's a wrinkle I initially missed: if the enclave > hasn't gone through ECREATE, encl->base is garbage. So either sgx_mmap() needs > to assume the !CREATED is creating a mismatched offset, or sgx_encl_create() needs > to iterate over and check all VMAs. > > Since there are advantages to usuing mmap(NULL) to find ELRANGE, IMO your best > option is to do the below. And then that mostly answers the question about > using mmap(/dev/sgx_enclave) to reserve ELRANGE, i.e. if userspace wants to use > fallocate(), then it effectively *must not* use mmap(/dev/sgx_enclave). > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c > index 262f5fb18d74..63fb41da35aa 100644 > --- a/arch/x86/kernel/cpu/sgx/driver.c > +++ b/arch/x86/kernel/cpu/sgx/driver.c > @@ -94,6 +94,11 @@ static int sgx_mmap(struct file *file, struct vm_area_struct *vma) > if (ret) > return ret; > > + if (!test_bit(SGX_ENCL_CREATED, &encl->flags) || > + vma->vm_pgoff != PFN_DOWN(vma->vm_start - encl->base)) > + encl->has_mismatched_offsests = true; > + > + > vma->vm_ops = &sgx_vm_ops; > vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO); > vma->vm_private_data = encl; > It appears we don't need to mark "has_mismatched_offsets" as true if userspace uses mmap(/dev/sgx_enclave) to reserve ELRANGE? Enclave's base isn't established yet, so theoretically, from enclave's perspective, we cannot say it has a mismatched offset. However we may want to return -EINVAL if userspace passes a non-zero pgoff in the mmap(/dev/sgx_enclave) to reserve ELRANGE. Anyway reserving ELRANGE isn't the big point. The SGX driver ABI is. Let's forget about reserving ELRANGE in below context, i.e., all mmap() below means non-ELRANGE-reserve mmap(/dev/sgx_enclave) :) I think yes marking enclave as "has_mismatched_offsets" works if userspace wants to use file-based ABIs (fadvice(), etc) for enclave, but this effectively means Haitao needs to modify _ALL_ existing mmap()s to pass the correct pgoff in order to use fadvice() for EAUG. Therefore, it seems more like we are changing ABI (or having a new ABI) to: Userspace must pass the correct pgoff to all mmaps() in order to use file-based ABIs for SGX. ( Because I think other drivers/subsystems doesn't have such limitation. For instance, for a normal file, userspace can have two mmap()s mapping to the same offset of the same file but with different address, and fadvice() should work for both mappings. A VMA-based "has_mismatched_offset" seems more reasonable for SGX but as I mentioned previously it may not easy to do from SGX driver. ) So I am not sure whether this belongs to "breaking existing ABI"? If the current ABI is we _require_ pgoff being zero, then the "encl- >has_mismatched_offsets" seems is breaking this ABI. If the current ABI is we _ignore_ pgoff, then "encl->has_mismatched_offsets" isn't breaking existing ABI. But in this case why do we need this at all? SGX driver just follows the existing file-based ABIs: If userspace wants to use file-based ABIs for some VMA, it needs to pass the correct pgoff. If not, file-based ABIs won't work for that VMA, but your enclave may still work if you don't use file-based ABIs. Sorry for being noisy, but does above make sense?
