On Fri, Jan 15, 2021, jarkko@xxxxxxxxxx wrote:
> From: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Fixes: 1728ab54b4be ("x86/sgx: Add a page reclaimer")
> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Suggested-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> ---
> 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);
> mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
> kfree(encl_mm);
> }
> --
> 2.29.2
>
