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.88, mailfrom: ray.ni@intel.com) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by groups.io with SMTP; Tue, 03 Sep 2019 09:56:30 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Sep 2019 09:56:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,463,1559545200"; d="scan'208";a="184818883" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga003.jf.intel.com with ESMTP; 03 Sep 2019 09:56:29 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 3 Sep 2019 09:56:29 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 3 Sep 2019 09:56:29 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.32]) by shsmsx102.ccr.corp.intel.com ([169.254.2.113]) with mapi id 14.03.0439.000; Wed, 4 Sep 2019 00:56:27 +0800 From: "Ni, Ray" To: "Nikodem, Damian" , "devel@edk2.groups.io" CC: "Dong, Eric" , "You, Benjamin" , Laszlo Ersek , "Rusocki, Krzysztof" 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: AQHVYmhAMVzvMHIZdkqISZLjczppTacaK7lQ Date: Tue, 3 Sep 2019 16:56:26 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C2BE9FA@SHSMSX104.ccr.corp.intel.com> References: <20190903145732.18604-1-damian.nikodem@intel.com> In-Reply-To: <20190903145732.18604-1-damian.nikodem@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYjc2YTNhYjAtZDEyYS00MzgyLWIxMGYtZDVkNGVkNWRjNmExIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiRDM4elkwWGtXcWczRUY3SDhZMm1wNjVkRDdsOFRmVys1bndaZ1pWUmNWdlwvNWNGZlhKSktqVG1SZ3ZLQ0cyUXgifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 re= duce 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 ; Dong, Eric ; Ni, Ray ; You, > Benjamin ; Laszlo Ersek ; Ruso= cki, Krzysztof > Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between APHand= ler's release of Busy spinlock and user- > triggered SmmStartupThisAP's >=20 > Race condition between APHandler's release of Busy spinlock and > user-triggered SmmStartupThisAP's acquisition attempt of the Busy spinloc= k (non-blocking mode). >=20 > 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. >=20 > 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) >=20 > << initial state >> >=20 > AcquireSpinLock (UserProcCompletion) > SmmStartupThisAp (Procedure) > AcquireSpinLockOrFail (Busy) > ReleaseSemaphore (Run) > UserProc () > DoStuff() DoSomeOtherStuff () >=20 > AcquireSpinLockOrFail (UserProcCompletion) AcquireSpinLockOrFail (UserPro= cCompletion) >=20 > ^^ waiting in a loop for user procedure's > completion =3D=3D these fail > ReleaseSpinLock (UserProcCompletion) AcquireSpinLockOrFail (UserPro= cCompletion) >=20 > ^^ this succeeds >=20 > ReleaseSpinLock (UserProcCompletion) >=20 > << return control to the caller and > reenter the flow >>> >=20 > AcquireSpinLock (UserProcCompletion) > SmmStartupThisAp (Procedure) > AcquireSpinLockOrFail (Busy) > ^^ this wins the race with AP's > ReleaseSpinLock and fails; > ReleaseSpinLock (Busy) > return EFI_INVALID_PARAMETER; >=20 > To remedy, if AcquireSpinLockOrFail (of the Busy spinlock) fails, perform= regular AcquireSpinLock -- this eliminates the race > condition. >=20 > Signed-off-by: Damian Nikodem > Cc: Eric Dong > Cc: Ray Ni > Cc: Benjamin You > Cc: Laszlo Ersek > Cc: Krzysztof Rusocki >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuD= xeSmm/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); > } >=20 > *Token =3D (MM_COMPLETION) CreateToken ();