Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between APHandler's release of Busy spinlock and user-triggered SmmStartupThisAP's

["Dong, Eric" <eric.dong@intel.com>]

Agree with Ray, no need to call AcquireSpinLockOrFail anymore. I think final code like change like below:

-  if (Token == NULL) {
-    AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
-  } else {
-    if (!AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy)) {
-      DEBUG((DEBUG_ERROR, "Can't acquire mSmmMpSyncData->CpuData[%d].Busy\n", CpuIndex));
-      return EFI_NOT_READY;
-    }
+  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
 
+  if (Token != NULL) {

Thanks,
Eric

> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, September 4, 2019 12:56 AM
> To: Nikodem, Damian <damian.nikodem@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; You, Benjamin
> <benjamin.you@intel.com>; Laszlo Ersek <lersek@redhat.com>; Rusocki,
> Krzysztof <krzysztof.rusocki@intel.com>
> Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between
> APHandler's release of Busy spinlock and user-triggered SmmStartupThisAP's
> 
> 1. can we directly call AcquireSpinLock()? *OrFail() can be removed IMO.
> 2. It's a patch to change the behavior of SmmStartupThisAP(). So that to
> reduce the potential bugs in caller's code. Patch title is a bit mis-leading.
> 
> > -----Original Message-----
> > From: Nikodem, Damian
> > Sent: Tuesday, September 3, 2019 7:58 AM
> > To: devel@edk2.groups.io
> > Cc: Nikodem, Damian <damian.nikodem@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; You, Benjamin
> > <benjamin.you@intel.com>; Laszlo Ersek <lersek@redhat.com>; Rusocki,
> > Krzysztof <krzysztof.rusocki@intel.com>
> > Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between
> > APHandler's release of Busy spinlock and user- triggered
> > SmmStartupThisAP's
> >
> > Race condition between APHandler's release of Busy spinlock and
> > user-triggered SmmStartupThisAP's acquisition attempt of the Busy spinlock
> (non-blocking mode).
> >
> > UserProc is the user's procedure to execute on an AP.
> > UserProcCompletion is the user procedure's completion spinlock.
> > All other variables are from EDK2.
> >
> > BSP                                          AP
> >
> ===============================================================
> ======================
> >
> 		 APHandler ()
> >
> 		   WaitForSemaphore (Run)
> >
> > 								   << initial
> state >>
> >
> > AcquireSpinLock (UserProcCompletion)
> > SmmStartupThisAp (Procedure)
> >   AcquireSpinLockOrFail (Busy)
> >   ReleaseSemaphore (Run)
> >
> 		   UserProc ()
> > DoStuff()                                        DoSomeOtherStuff ()
> >
> > AcquireSpinLockOrFail (UserProcCompletion) AcquireSpinLockOrFail
> > (UserProcCompletion)
> >
> > ^^ waiting in a loop for user procedure's
> >    completion == these fail
> > ReleaseSpinLock (UserProcCompletion)       AcquireSpinLockOrFail
> (UserProcCompletion)
> >
> > ^^ this succeeds
> >
> > ReleaseSpinLock (UserProcCompletion)
> >
> > << return control to the caller and
> >    reenter the flow >>>
> >
> > AcquireSpinLock (UserProcCompletion)
> > SmmStartupThisAp (Procedure)
> >   AcquireSpinLockOrFail (Busy)
> >   ^^ this wins the race with AP's
> > 	 ReleaseSpinLock and fails;
> >
> 		   ReleaseSpinLock (Busy)
> > 	return EFI_INVALID_PARAMETER;
> >
> > To remedy, if AcquireSpinLockOrFail (of the Busy spinlock) fails,
> > perform regular AcquireSpinLock -- this eliminates the race condition.
> >
> > Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Benjamin You <benjamin.you@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Krzysztof Rusocki <krzysztof.rusocki@intel.com>
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index d8d2b6f444..206e196a76 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -1239,8 +1239,16 @@ InternalSmmStartupThisAp (
> >      AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> >    } else {
> >      if (!AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy))
> {
> > -      DEBUG((DEBUG_ERROR, "Can't acquire mSmmMpSyncData-
> >CpuData[%d].Busy\n", CpuIndex));
> > -      return EFI_NOT_READY;
> > +      DEBUG ((DEBUG_INFO, "BSP[%d] finds AP[%d] busy at proc 0x%llX
> (param 0x%llX), ",
> > +        mSmmMpSyncData->BspIndex,
> > +        CpuIndex,
> > +        *mSmmMpSyncData->CpuData[CpuIndex].Procedure,
> > +        (VOID*)mSmmMpSyncData->CpuData[CpuIndex].Parameter));
> > +      DEBUG ((DEBUG_INFO, "new proc 0x%llX (param 0x%llX). Waiting for
> the previous AP procedure to complete...\n",
> > +        Procedure,
> > +        ProcArguments));
> > +
> > +      AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> >      }
> >
> >      *Token = (MM_COMPLETION) CreateToken ();