Re: [PATCH v9 3/3] x86/sgx: Fine grained SGX MCA behavior for virtualization

From: "jarkko@kernel.org" <jarkko@kernel.org>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: "Hansen, Dave" <dave.hansen@intel.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"Christopherson,, Sean" <seanjc@google.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"bp@suse.de" <bp@suse.de>, "Zhang, Cathy" <cathy.zhang@intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"Luck, Tony" <tony.luck@intel.com>, "Du, Fan" <fan.du@intel.com>,
	"Li, Zhiquan1" <zhiquan1.li@intel.com>
Subject: Re: [PATCH v9 3/3] x86/sgx: Fine grained SGX MCA behavior for virtualization
Date: Tue, 1 Nov 2022 02:53:43 +0200	[thread overview]
Message-ID: <Y2BuF1juF0R0hhq0@kernel.org> (raw)
In-Reply-To: <31afc45e67f303645b81d231e4a5cb6878b30016.camel@intel.com>

On Mon, Oct 24, 2022 at 10:23:10PM +0000, Huang, Kai wrote:
> On Sun, 2022-10-23 at 23:39 +0300, jarkko@kernel.org wrote:
> > On Wed, Oct 19, 2022 at 10:59:20AM +0000, Huang, Kai wrote:
> > > static int __sgx_vepc_fault(struct sgx_vepc *vepc,
> > >                             struct vm_area_struct *vma, unsigned long addr)
> > > {
> > > 	......
> > >         /* Calculate index of EPC page in virtual EPC's page_array */
> > >         index = vma->vm_pgoff + PFN_DOWN(addr - vma->vm_start);
> > > 
> > >         epc_page = xa_load(&vepc->page_array, index);
> > >         if (epc_page)
> > >                 return 0;
> > > 
> > > 	...
> > > }
> > > 
> > > As you can see if the EPC page has already been populated at a given index of
> > > one virtual EPC instance, the current fault handler just assumes the mapping is
> > > already there and returns success immediately.  This causes a bug when one
> > > virtual EPC instance is shared by multi processes via fork(): if the EPC page at
> > > one index is already populated by the parent process, when the child accesses
> > > the same page using different virtual address, the fault handler just returns
> > > success w/o actually setting up the mapping for the child, resulting in endless
> > > page fault.
> > > 
> > > This needs to be fixed in no matter what way.
> > 
> > I think you mean that vm_insert_pfn() does not happen for child because
> > of early return?
> > 
> 
> Yes exactly.  Sorry for not pointing out directly.

Np.

> 
> > I did not understand the part about "different virtual
> > addresses", as it is the same mapping.
> 
> The child can use mmap() to get a new mapping.  Whether the virtual address is
> different from the parent's doesn't matter actually.

Thanks for the response, I had one additional query responded to Zhiquan.

BR, Jarkko