From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: krzysztof.rusocki@intel.com) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by groups.io with SMTP; Wed, 04 Sep 2019 05:14:17 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Sep 2019 05:14:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,465,1559545200"; d="scan'208";a="190154560" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by FMSMGA003.fm.intel.com with ESMTP; 04 Sep 2019 05:14:16 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.21]) by IRSMSX109.ger.corp.intel.com ([169.254.13.11]) with mapi id 14.03.0439.000; Wed, 4 Sep 2019 13:14:15 +0100 From: "Rusocki, Krzysztof" To: "Ni, Ray" , "Nikodem, Damian" , "devel@edk2.groups.io" CC: "Dong, Eric" , "You, Benjamin" , Laszlo Ersek Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between APHandler's release of Busy spinlock and user-triggered SmmStartupThisAP's Thread-Topic: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between APHandler's release of Busy spinlock and user-triggered SmmStartupThisAP's Thread-Index: AQHVYmhAz5j8qqZanUWIasRBkHmPMKcaG3cAgAASqtA= Date: Wed, 4 Sep 2019 12:14:14 +0000 Message-ID: <18D57D943432D54EBBAF26972EED48A738E65486@IRSMSX104.ger.corp.intel.com> References: <20190903145732.18604-1-damian.nikodem@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C2BE9FA@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C2BE9FA@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYjc2YTNhYjAtZDEyYS00MzgyLWIxMGYtZDVkNGVkNWRjNmExIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiRDM4elkwWGtXcWczRUY3SDhZMm1wNjVkRDdsOFRmVys1bndaZ1pWUmNWdlwvNWNGZlhKSktqVG1SZ3ZLQ0cyUXgifQ== dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [163.33.239.180] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable From: Ni, Ray = Sent: Tuesday, September 3, 2019 6:56 PM > 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-leadin= g. The way I see it, it's currently a mix of both: a) catching issues w/caller's code (user calling SmmStartupThisAp while AP = executing previous procedure) and b) tackling the deficiency of the API because the caller has no 100% confid= ent way of determining if non-blocking AP execution has completed or not [!= !] I agree it's not the implementation responsibility to hand-hold the user (a= t least not beyond returning the error code). That said, I don't see an issue dropping the debug message and just using s= ingle call to AcquireSpinLock. However, I do want to emphasize, the primary intent of the change is to dea= l with point (b) rather than (a) :-) Thanks, Krzysztof > -----Original Message----- > From: Nikodem, Damian > Sent: Tuesday, September 3, 2019 7:58 AM > To: devel@edk2.groups.io > Cc: Nikodem, Damian ; Dong, Eric = > ; Ni, Ray ; You, Benjamin = > ; Laszlo Ersek ; Rusocki, = > Krzysztof > 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 spinloc= k (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 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 =3D=3D these fail > ReleaseSpinLock (UserProcCompletion) AcquireSpinLockOrFail (UserPro= cCompletion) > = > ^^ 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 > Cc: Eric Dong > Cc: Ray Ni > Cc: Benjamin You > Cc: Laszlo Ersek > Cc: Krzysztof Rusocki > = > 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].Bus= y\n", CpuIndex)); > - return EFI_NOT_READY; > + DEBUG ((DEBUG_INFO, "BSP[%d] finds AP[%d] busy at proc 0x%llX (par= am 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 t= he previous AP procedure to complete...\n", > + Procedure, > + ProcArguments)); > + > + AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); > } > = > *Token =3D (MM_COMPLETION) CreateToken (); -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydz= ial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-31= 6 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata= i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wi= adomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiek= olwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the s= ole use of the intended recipient(s). If you are not the intended recipient= , please contact the sender and delete all copies; any review or distributi= on by others is strictly prohibited.