| Message ID | 20210615101639.291929-1-kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed | expand |
On Tue, Jun 15, 2021 at 10:16:39PM +1200, Kai Huang wrote: > xa_destroy() needs to be called to destroy virtual EPC's page arra y > before calling kfree() to free the virtual EPC. Currently it is not > calaled. Add the missing xa_destroy() to fix. called > Fixes: 540745ddbc70 ("x86/sgx: Introduce virtual EPC for use by KVM guests") > Tested-by: Yang Zhong <yang.zhong@intel.com> > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > arch/x86/kernel/cpu/sgx/virt.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c > index 6ad165a5c0cc..64511c4a5200 100644 > --- a/arch/x86/kernel/cpu/sgx/virt.c > +++ b/arch/x86/kernel/cpu/sgx/virt.c > @@ -212,6 +212,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file) > list_splice_tail(&secs_pages, &zombie_secs_pages); > mutex_unlock(&zombie_secs_pages_lock); > > + xa_destroy(&vepc->page_array); > kfree(vepc); > > return 0; > -- > 2.31.1 > > /Jarkko
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? Also, is this the kind of thing that a simple open/add/close selftest might have found? Maybe we should beef up the selftests a bit. Acked-by: Dave Hansen <dave.hansen@intel.com>
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@intel.com> Thank you!
On Tue, 2021-06-15 at 16:20 +0300, Jarkko Sakkinen wrote: > On Tue, Jun 15, 2021 at 10:16:39PM +1200, Kai Huang wrote: > > xa_destroy() needs to be called to destroy virtual EPC's page arra > y > > > before calling kfree() to free the virtual EPC. Currently it is not > > calaled. Add the missing xa_destroy() to fix. > called Thanks Jarkko. I literally need to find some way to avoid such error in future :) > > > Fixes: 540745ddbc70 ("x86/sgx: Introduce virtual EPC for use by KVM guests") > > Tested-by: Yang Zhong <yang.zhong@intel.com> > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > arch/x86/kernel/cpu/sgx/virt.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c > > index 6ad165a5c0cc..64511c4a5200 100644 > > --- a/arch/x86/kernel/cpu/sgx/virt.c > > +++ b/arch/x86/kernel/cpu/sgx/virt.c > > @@ -212,6 +212,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file) > > list_splice_tail(&secs_pages, &zombie_secs_pages); > > mutex_unlock(&zombie_secs_pages_lock); > > > > + xa_destroy(&vepc->page_array); > > kfree(vepc); > > > > return 0; > > -- > > 2.31.1 > > > > > > /Jarkko
On Wed, Jun 16, 2021 at 12:30:04PM +1200, Kai Huang wrote:
> Thanks Jarkko. I literally need to find some way to avoid such error in future :)
That way is called "integrate checkpatch.pl into your patch creation
workflow".
On Thu, 2021-06-17 at 16:34 +0200, Borislav Petkov wrote: > On Wed, Jun 16, 2021 at 12:30:04PM +1200, Kai Huang wrote: > > Thanks Jarkko. I literally need to find some way to avoid such error in future :) > > That way is called "integrate checkpatch.pl into your patch creation > workflow". > Thanks for suggestion. Yes I actually did the checkpatch.pl, but it didn't report typo in commit message. A little bit strange.
On Fri, Jun 18, 2021 at 12:04:55PM +1200, Kai Huang wrote: > Thanks for suggestion. Yes I actually did the checkpatch.pl, but it > didn't report typo in commit message. A little bit strange. Yah, and I know it does catch typos. It seems it does so only sometimes: $ ./scripts/checkpatch.pl --strict /tmp/kai.01 total: 0 errors, 0 warnings, 0 checks, 7 lines checked /tmp/kai.01 has no obvious style problems and is ready for submission. Someday soon we'll have a better way to deal with this.
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c index 6ad165a5c0cc..64511c4a5200 100644 --- a/arch/x86/kernel/cpu/sgx/virt.c +++ b/arch/x86/kernel/cpu/sgx/virt.c @@ -212,6 +212,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file) list_splice_tail(&secs_pages, &zombie_secs_pages); mutex_unlock(&zombie_secs_pages_lock); + xa_destroy(&vepc->page_array); kfree(vepc); return 0;
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. Fixes: 540745ddbc70 ("x86/sgx: Introduce virtual EPC for use by KVM guests") Tested-by: Yang Zhong <yang.zhong@intel.com> Signed-off-by: Kai Huang <kai.huang@intel.com> --- arch/x86/kernel/cpu/sgx/virt.c | 1 + 1 file changed, 1 insertion(+)