Re: [PATCH v4] x86/sgx: Fix the call order of synchronize_srcu() in sgx_release()

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Jan 25, 2021 at 07:49:04AM -0800, Dave Hansen wrote:
> Haitao managed to create another splat over the weekend.  It was, indeed:
> 
> > WARNING: CPU: 3 PID: 7620 at kernel/rcu/srcutree.c:374 cleanup_srcu_struct+0xed/0x100
> 
> which is:
> 
> >         if (WARN_ON(!srcu_get_delay(ssp)))
> >                 return; /* Just leak it! */
> 
> That check means that there is an outstanding "expedited" grace period.
>  The fact that it's expedited is not important.  This:
> 
> 	https://lwn.net/Articles/202847/
> 
> describes the reasoning behind the warning:
> 
> 	If the struct srcu_struct is dynamically allocated, then
> 	cleanup_srcu_struct() must be called before it is freed ... the
> 	caller must take care to ensure that all SRCU read-side critical
> 	sections have completed (and that no more will commence) before
> 	calling cleanup_srcu_struct().
> 
> synchronize_srcu() will (obviously) wait for the grace period to
> complete.  Calling it will shut up the warning for sure, most of the time.
> 
> The required sequence of events is in here:
> 
> > https://lore.kernel.org/lkml/1492472726-3841-4-git-send-email-paulmck@linux.vnet.ibm.com/

I'll add "Link:" for this to the final fix. It's a good reference.

> I suspect that the mmu notifier's synchronize_srcu() is run in parallel
> very close to when cleanup_srcu_struct() is called.  This violates the
> "prevent any further calls to synchronize_srcu" rule.
> 
> So, while I suspect that adding a synchronize_srcu() is *part* of the
> correct solution, I'm still not convinced that the
> sgx_mmu_notifier_release() code is correct.

What Sean suggested in private discussion, i.e. using kref_get() in the MMU
notifier AFAIK should be enough to do the necessary serialization.

/Jarkko