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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Jan 18, 2021 at 07:57:12PM +0100, Borislav Petkov wrote:
> On Sat, Jan 16, 2021 at 07:12:54AM +0200, Jarkko Sakkinen wrote:
> > > https://lkml.kernel.org/r/X/zoarV7gd/LNo4A@kernel.org
> > 
> > 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?
> 
> Not sure.
> 
> That link above says:
> 
> "Now, let's imagine that there is exactly one entry in the encl->mm_list.
> and sgx_release() execution gets scheduled right after returning from
> synchronize_srcu().
> 
> With some bad luck, some process comes and removes that last entry befoe
> sgx_release() acquires mm_lock."
> 
> So, the last entry gets removed by some other process before
> sgx_release() acquires mm_lock. When it does acquire that lock, the test
> 
> 	if (list_empty(&encl->mm_list))
> 
> will be true because "some other process" has removed that last entry.
> 
> So why do you need the synchronize_srcu() call when this process sees an
> empty mm_list already?
> 
> Thx.

The other process aka some process using the enclave calls list_del_rcu()
(and synchronize_srcu()), which starts a new grace period. If we don't
do it, then the cleanup_srcu() will race with that grace period.

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

/Jarkko