On Fri, Jan 15, 2021 at 08:18:09AM +0100, Borislav Petkov wrote: > On Fri, Jan 15, 2021 at 03:46:38AM +0200, 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(). > > To fix what? > > Lemme explain one more time: a commit message for a race condition > fix needs to explain in *detail* what the race condition is. And that > explanation needs to be understandable months and years from now. > > You have the function call order above, now you have to explain what can > happen. Just how you did here: > > https://lkml.kernel.org/r/X/zoarV7gd/LNo4A@xxxxxxxxxx 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? > > 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() > > "The call is need"? > > > $ git grep sgx_encl_mmu_notifier_release > $ > > WTF? > > Please be more careful. Note taken. > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette /Jarkko
