On Fri, Jan 15, 2021 at 08:18:09AM +0100, Borislav Petkov wrote:
On Fri, Jan 15, 2021 at 03:46:38AM +0200, jarkko@kernel.org wrote:
From: Jarkko Sakkinen jarkko@kernel.org
The most trivial example of a race condition can be demonstrated with this example where mm_list contains just one entry:
CPU A CPU B sgx_release() sgx_mmu_notifier_release() list_del_rcu() sgx_encl_release() synchronize_srcu() cleanup_srcu_struct()
To fix this, call synchronize_srcu() before checking whether mm_list is empty in sgx_release().
To fix what?
Lemme explain one more time: a commit message for a race condition fix needs to explain in *detail* what the race condition is. And that explanation needs to be understandable months and years from now.
You have the function call order above, now you have to explain what can happen. Just how you did here:
OK, I could recall the race that from but that must be partly because I've been proactively working on it, i.e. getting your point.
So let's say I add this after the sequence:
"The sequence demonstrates a scenario where CPU B starts a new grace period, which goes unnoticed by CPU A in sgx_release(), because it did not remove the final entry from the enclave's mm list."
Would this be sufficient or not?
Cc: stable@vger.kernel.org Fixes: 1728ab54b4be ("x86/sgx: Add a page reclaimer") Suggested-by: Sean Christopherson seanjc@google.com Suggested-by: Haitao Huang haitao.huang@linux.intel.com Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
v4:
- Rewrite the commit message.
- Just change the call order. *_expedited() is out of scope for this bug fix.
v3: Fine-tuned tags, and added missing change log for v2. v2: Switch to synchronize_srcu_expedited(). arch/x86/kernel/cpu/sgx/driver.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c index f2eac41bb4ff..53056345f5f8 100644 --- a/arch/x86/kernel/cpu/sgx/driver.c +++ b/arch/x86/kernel/cpu/sgx/driver.c @@ -65,11 +65,16 @@ static int sgx_release(struct inode *inode, struct file *file) spin_unlock(&encl->mm_lock);
/** The call is need even if the list empty, because sgx_encl_mmu_notifier_release()"The call is need"?
$ git grep sgx_encl_mmu_notifier_release $
WTF?
Please be more careful.
Note taken.
-- Regards/Gruss, Boris.
/Jarkko