Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Kai Huang]

On Tue, 2021-06-15 at 08:39 -0700, Dave Hansen wrote:
> On 6/15/21 3:16 AM, Kai Huang wrote:
> > xa_destroy() needs to be called to destroy virtual EPC's page arra
> > before calling kfree() to free the virtual EPC.  Currently it is not
> > calaled.  Add the missing xa_destroy() to fix.
> 
> Looks good Kai, thanks for fixing this.
> 
> Could you please take a good look through the sgx_release() and the vpec
> equivalent and see if anything else stands out as possibly being missed?

I looked over.  One potential issue is both sgx_encl and sgx_vepc have 'struct mutex lock'
embedded,  but mutex_destroy() is not called when they are released.  However I am not
sure whether this is worth fixing, since mutex_destroy() is empty unless
CONFIG_DEBUG_MUTEXES is turned on (even with it turned on, mutex_destroy() doesn't do
anything like freeing resources so in practice there should have no problem).

Another thing is sgx_encl_release() doesn't explicitly call xa_erase() for each EPC page
in encl->page_array when looping it over to free all EPC pages, but I think it is OK since
xa_destroy() is called later which will destroy all xarray internal data structures.  But
I don't know internal implementation of xarray.

>  Also, is this the kind of thing that a simple open/add/close selftest
> might have found?

It might be useful but I don't think it can detect things like xa_destroy() being missing.

> 
> Maybe we should beef up the selftests a bit.
> 
> Acked-by: Dave Hansen <dave.hansen@xxxxxxxxx>

Thank you!