Re: [PATCH 1/6] x86/sgx: Do not consider unsanitized pages an error

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

On Wed, 2022-08-31 at 05:15 +0300, jarkko@kernel.org wrote:
> On Wed, Aug 31, 2022 at 01:27:58AM +0000, Huang, Kai wrote:
> > On Tue, 2022-08-30 at 15:54 -0700, Reinette Chatre wrote:
> > > Hi Jarkko,
> > > 
> > > On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> > > > In sgx_init(), if misc_register() for the provision device fails, and
> > > > neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
> > > > prematurely stopped.
> > > 
> > > I do not think misc_register() is required to fail for the scenario to
> > > be triggered (rather use "or" than "and"?). Perhaps just
> > > "In sgx_init(), if a failure is encountered after ksgxd is started
> > > (via sgx_page_reclaimer_init()) ...".
> > 
> > IMHO "a failure" might be too vague.  For instance, failure to sgx_drv_init()
> > won't immediately result in ksgxd to stop prematurally.  As long as KVM SGX can
> > be initialized successfully, sgx_init() still returns 0.
> > 
> > Btw I was thinking whether we should move sgx_page_reclaimer_init() to the end
> > of sgx_init(), after we make sure at least one of the driver and the KVM SGX is
> > initialized successfully.  Then the code change in this patch won't be necessary
> > if I understand correctly.  AFAICT there's no good reason to start the ksgxd at
> > early stage before we are sure either the driver or KVM SGX will work.
> 
> I would focus fixing the existing flow rather than reinventing the flow.
> 
> It can be made to work, and therefore it is IMHO correct action to take.

From another perspective, the *existing flow* is the reason which causes this
bug.  A real fix is to fix the flow itself.

> 
> > Btw currently EPC pages assigned to KVM guest cannot be reclaimed, so
> > theoretically ksgxd can be moved to sgx_drv_init(), but who knows someday we
> > will decide to make KVM guest EPC pages to be able to be reclaimed. :)
> 
> I'm open to changes but it is in my opinion out of context for this.
> 
> 

Yeah.  I was expressing the reason I suggested to move sgx_page_reclaimer_init()
to the end of sgx_init(), but not to sgx_drv_init().

But moving to sgx_drv_init() also makes sense to me given KVM guest EPC pages
are not reclaimable now.  For now there's no reason to run ksgxd() if only
virtual EPC driver is enabled.  We can move sgx_page_reclaimer_init() out of
sgx_drv_init() when we add KVM guest EPC page reclaiming support (if it happens
in the future).

-- 
Thanks,
-Kai