Re: [PATCH] x86/sgx: Synchronize encl->srcu in sgx_encl_release().

[Sean Christopherson]

On Fri, Dec 11, 2020, Jarkko Sakkinen wrote:
> Each sgx_mmun_notifier_release() starts a grace period, which means that

Should be sgx_mmu_notifier_release(), here and in the comment.

> one extra synchronize_rcu() in sgx_encl_release(). Add it there.
> 
> sgx_release() has the loop that drains the list but with bad luck the
> entry is already gone from the list before that loop processes it.

Why not include the actual analysis that "proves" the bug?  The splat that
Haitao reported would also be useful info.

> Fixes: 1728ab54b4be ("x86/sgx: Add a page reclaimer")
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Reported-by: Sean Christopherson <seanjc@xxxxxxxxxx>

Haitao reported the bug, and for all intents and purposes provided the fix.  I
just did the analysis to verify that there was a legitimate bug and that the
synchronization in sgx_encl_release() was indeed necessary.

> Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/sgx/encl.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index ee50a5010277..48539a6ee315 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -438,6 +438,13 @@ void sgx_encl_release(struct kref *ref)
>  	if (encl->backing)
>  		fput(encl->backing);
>  
> +	/*
> +	 * Each sgx_mmun_notifier_release() starts a grace period. Thus one
> +	 * "extra" synchronize_rcu() is required here. This can go undetected by
> +	 * sgx_release() when it drains the mm list.
> +	 */
> +	synchronize_srcu(&encl->srcu);
> +
>  	cleanup_srcu_struct(&encl->srcu);
>  
>  	WARN_ON_ONCE(!list_empty(&encl->mm_list));
> -- 
> 2.27.0
>