On Fri, Jan 15, 2021, 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().
Why haven't you included the splat that Haitao provided? That would go a long way to helping answer Boris' question about exactly what is broken...
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()* could have initiated a new grace period.*/synchronize_srcu(&encl->srcu);
I don't think this completely fixes the issue as sgx_release() isn't guaranteed to trigger cleanup_srcu_struct(), e.g. the reclaimer can also have a reference to the enclave.
- /* The enclave is no longer mapped by any mm. */ if (!encl_mm) break;
synchronize_srcu(&encl->srcu);-- 2.29.2