On Fri, 15 Jan 2021 13:45:21 -0800 Sean Christopherson wrote: > On Sat, Jan 16, 2021, Kai Huang wrote: > > On Fri, 15 Jan 2021 07:39:44 -0800 Dave Hansen wrote: > > > On 1/15/21 6:07 AM, Kai Huang wrote: > > > >>From virtual EPC's perspective, if we don't force this in kernel, then > > > > *theoretically*, userspace can use fork() to make multiple VMs map to the > > > > same physical EPC, which will potentially cause enclaves in all VMs to behave > > > > abnormally. So to me, from this perspective, it's better to enforce in kernel > > > > so that only first VM can use this virtual EPC instance, because EPC by > > > > architectural design cannot be shared. > > > > > > > > But as Sean said, KVM doesn't support VM across multiple mm structs. And if I > > > > read code correctly, KVM doesn't support userspace to use fork() to create new > > > > VM. For instance, when creating VM, KVM grabs current->mm and keeps it in > > > > 'struct kvm' for bookkeeping, and kvm_vcpu_ioctl() and kvm_device_ioctl() will > > > > refuse to work if kvm->mm doesn't equal to current->mm. So in practice, I > > > > believe w/o enforcing this in kernel, we should also have no problem here. > > > > > > > > Sean, please correct me if I am wrong. > > > > > > > > Dave, if above stands, do you think it is reasonable to keep current->mm in > > > > epc->mm and enforce in sgx_virt_epc_mmap()? > > > > > > Everything you wrote above tells me the kernel should not be enforcing > > > the behavior. You basically said that it's only a theoretical problem, > > > and old if someone goes and does something with KVM that's nobody can do > > > today. > > > > > > You've 100% convinced me that having the kernel enforce this is > > > *un*reasonable. > > > > Sean, I'll remove epc->mm, unless I see your further objection. > > It's probably ok. I guess worst case scenario, to avoid the mm tracking > nightmare for oversubscription, you could prevent attaching KVM to a virtual EPC > if there is already a mm associated with the EPC, or if there are already EPC > pages "in" the virt EPC. Since we are not 100% certain oversubscription will be upstreamed, I think it makes sense to address when we do it. For now, let us just drop it. Makes sense? Thanks.
