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

From: Borislav Petkov <bp@alien8.de>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: linux-sgx@vger.kernel.org, dave.hansen@intel.com,
	kai.huang@intel.com, haitao.huang@intel.com, seanjc@google.com,
	stable@vger.kernel.org,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	Jethro Beekman <jethro@fortanix.com>
Subject: Re: [PATCH v4] x86/sgx: Fix the call order of synchronize_srcu() in sgx_release()
Date: Mon, 18 Jan 2021 19:57:12 +0100	[thread overview]
Message-ID: <20210118185712.GE30090@zn.tnic> (raw)
In-Reply-To: <YAJ11v5tuS2uMuNm@kernel.org>

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette