Hi Dandan, Thanks for the comment. Patch v2 is just sent with the update. Please help to review.
Thank you, Ma Hua > -----Original Message----- > From: Bi, Dandan <dandan...@intel.com> > Sent: Tuesday, October 12, 2021 12:13 PM > To: Ma, Hua <hua...@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.w...@intel.com>; Liming Gao > <gaolim...@byosoft.com.cn>; Ni, Ray <ray...@intel.com> > Subject: RE: [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when > iterating gHandleList > > Hi Hua, > > One minor comment inline, please check. > > > > Thanks, > Dandan > > > -----Original Message----- > > From: Ma, Hua <hua...@intel.com> > > Sent: Monday, October 11, 2021 6:45 PM > > To: devel@edk2.groups.io > > Cc: Ma, Hua <hua...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; > > Liming Gao <gaolim...@byosoft.com.cn>; Bi, Dandan > <dandan...@intel.com>; > > Ni, Ray <ray...@intel.com> > > Subject: [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating > > gHandleList > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680 > > > > This patch fixes the following issue: > > > > The global variable gHandleList is a linked list. > > This list is locked when a entry is added or removed from the list, but > > there > is no > > lock when iterating this list in function CoreValidateHandle(). > > It can lead to "Handle.c (76): CR has Bad Signature" assertion if the > > iterated > > entry in the list is just removed by other task during iterating. > > Locking the list when iterating can fix this issue. > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Dandan Bi <dandan...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Signed-off-by: Hua Ma <hua...@intel.com> > > --- > > MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++--- > > MdeModulePkg/Core/Dxe/Hand/Handle.c | 47 ++++++++++++++------ > -- > > MdeModulePkg/Core/Dxe/Hand/Handle.h | 3 +- > > MdeModulePkg/Core/Dxe/Hand/Notify.c | 2 +- > > 4 files changed, 39 insertions(+), 23 deletions(-) > > > > diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c > > b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c > > index feabf12faf..eb8a765d2c 100644 > > --- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c > > +++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c > > @@ -68,7 +68,7 @@ CoreConnectController ( > > // > > // Make sure ControllerHandle is valid > > // > > - Status = CoreValidateHandle (ControllerHandle); > > + Status = CoreValidateHandle (ControllerHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -154,7 +154,7 @@ CoreConnectController ( > > // > > // Make sure the DriverBindingHandle is valid > > // > > - Status = CoreValidateHandle (ControllerHandle); > > + Status = CoreValidateHandle (ControllerHandle, TRUE); > > if (EFI_ERROR (Status)) { > > // > > // Release the protocol lock on the handle database @@ -268,7 +268,7 > @@ > > AddSortedDriverBindingProtocol ( > > // > > // Make sure the DriverBindingHandle is valid > > // > > - Status = CoreValidateHandle (DriverBindingHandle); > > + Status = CoreValidateHandle (DriverBindingHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return; > > } > > @@ -746,7 +746,7 @@ CoreDisconnectController ( > > // > > // Make sure ControllerHandle is valid > > // > > - Status = CoreValidateHandle (ControllerHandle); > > + Status = CoreValidateHandle (ControllerHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -755,7 +755,7 @@ CoreDisconnectController ( > > // Make sure ChildHandle is valid if it is not NULL > > // > > if (ChildHandle != NULL) { > > - Status = CoreValidateHandle (ChildHandle); > > + Status = CoreValidateHandle (ChildHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c > > b/MdeModulePkg/Core/Dxe/Hand/Handle.c > > index 6eccb41ecb..b4f389bb35 100644 > > --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c > > +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c > > @@ -55,31 +55,46 @@ CoreReleaseProtocolLock ( > > Check whether a handle is a valid EFI_HANDLE > > > > @param UserHandle The handle to check > > + @param IsLocked The protocol lock is acquried or not > > > > @retval EFI_INVALID_PARAMETER The handle is NULL or not a valid > > EFI_HANDLE. > > + @retval EFI_NOT_FOUND The handle is not found in the handle > > database. > > @retval EFI_SUCCESS The handle is valid EFI_HANDLE. > > > > **/ > > EFI_STATUS > > CoreValidateHandle ( > > - IN EFI_HANDLE UserHandle > > + IN EFI_HANDLE UserHandle, > > + IN BOOLEAN IsLocked > > ) > > { > > IHANDLE *Handle; > > LIST_ENTRY *Link; > > + EFI_STATUS Status; > > > > if (UserHandle == NULL) { > > return EFI_INVALID_PARAMETER; > > } > > > > + if (IsLocked) { > > + ASSERT_LOCKED(&gProtocolDatabaseLock); > > + } else { > > + CoreAcquireProtocolLock (); > > + } > > + > > + Status = EFI_NOT_FOUND; > > for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link- > >BackLink) { > > Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE); > > if (Handle == (IHANDLE *) UserHandle) { > > - return EFI_SUCCESS; > > + Status = EFI_SUCCESS; > > + break; > > } > > } > > > > - return EFI_INVALID_PARAMETER; > > + if (!IsLocked) { > > + CoreReleaseProtocolLock (); > > + } > > + return Status; > > } > > > > > > @@ -428,7 +443,7 @@ CoreInstallProtocolInterfaceNotify ( > > // > > InsertTailList (&gHandleList, &Handle->AllHandles); > > } else { > > - Status = CoreValidateHandle (Handle); > > + Status = CoreValidateHandle (Handle, TRUE); > > if (EFI_ERROR (Status)) { > > DEBUG((DEBUG_ERROR, "InstallProtocolInterface: input handle at 0x%x > is > > invalid\n", Handle)); > > goto Done; > > @@ -723,7 +738,7 @@ CoreUninstallProtocolInterface ( > > // > > // Check that UserHandle is a valid handle > > // > > - Status = CoreValidateHandle (UserHandle); > > + Status = CoreValidateHandle (UserHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -899,7 +914,7 @@ CoreGetProtocolInterface ( > > IHANDLE *Handle; > > LIST_ENTRY *Link; > > > > - Status = CoreValidateHandle (UserHandle); > > + Status = CoreValidateHandle (UserHandle, TRUE); > [Dandan] As current all callers of CoreGetProtocolInterface () has acquired > the lock, it's OK to pass TRUE to indicate the lock is acquired. > I am just thinking about following small question. > Could we add some comments to explain why it's true here and also notify > that caller should also acquire the lock firstly by itself if there is any > new caller > added to call this function? > > > if (EFI_ERROR (Status)) { > > return NULL; > > } > > @@ -1013,7 +1028,7 @@ CoreOpenProtocol ( > > // > > // Check for invalid UserHandle > > // > > - Status = CoreValidateHandle (UserHandle); > > + Status = CoreValidateHandle (UserHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -1023,11 +1038,11 @@ CoreOpenProtocol ( > > // > > switch (Attributes) { > > case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER : > > - Status = CoreValidateHandle (ImageHandle); > > + Status = CoreValidateHandle (ImageHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > - Status = CoreValidateHandle (ControllerHandle); > > + Status = CoreValidateHandle (ControllerHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -1037,17 +1052,17 @@ CoreOpenProtocol ( > > break; > > case EFI_OPEN_PROTOCOL_BY_DRIVER : > > case EFI_OPEN_PROTOCOL_BY_DRIVER | > EFI_OPEN_PROTOCOL_EXCLUSIVE : > > - Status = CoreValidateHandle (ImageHandle); > > + Status = CoreValidateHandle (ImageHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > - Status = CoreValidateHandle (ControllerHandle); > > + Status = CoreValidateHandle (ControllerHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > break; > > case EFI_OPEN_PROTOCOL_EXCLUSIVE : > > - Status = CoreValidateHandle (ImageHandle); > > + Status = CoreValidateHandle (ImageHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -1249,16 +1264,16 @@ CoreCloseProtocol ( > > // > > // Check for invalid parameters > > // > > - Status = CoreValidateHandle (UserHandle); > > + Status = CoreValidateHandle (UserHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > - Status = CoreValidateHandle (AgentHandle); > > + Status = CoreValidateHandle (AgentHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > if (ControllerHandle != NULL) { > > - Status = CoreValidateHandle (ControllerHandle); > > + Status = CoreValidateHandle (ControllerHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -1443,7 +1458,7 @@ CoreProtocolsPerHandle ( > > UINTN ProtocolCount; > > EFI_GUID **Buffer; > > > > - Status = CoreValidateHandle (UserHandle); > > + Status = CoreValidateHandle (UserHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h > > b/MdeModulePkg/Core/Dxe/Hand/Handle.h > > index 83eb2b9f3a..b962d80a29 100644 > > --- a/MdeModulePkg/Core/Dxe/Hand/Handle.h > > +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h > > @@ -251,7 +251,8 @@ CoreReleaseProtocolLock ( **/ EFI_STATUS > > CoreValidateHandle ( > > - IN EFI_HANDLE UserHandle > > + IN EFI_HANDLE UserHandle, > > + IN BOOLEAN IsLocked > > ); > > > > // > > diff --git a/MdeModulePkg/Core/Dxe/Hand/Notify.c > > b/MdeModulePkg/Core/Dxe/Hand/Notify.c > > index 553413a350..f4e7c01e96 100644 > > --- a/MdeModulePkg/Core/Dxe/Hand/Notify.c > > +++ b/MdeModulePkg/Core/Dxe/Hand/Notify.c > > @@ -188,7 +188,7 @@ CoreReinstallProtocolInterface ( > > PROTOCOL_INTERFACE *Prot; > > PROTOCOL_ENTRY *ProtEntry; > > > > - Status = CoreValidateHandle (UserHandle); > > + Status = CoreValidateHandle (UserHandle, FALSE); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > -- > > 2.32.0.windows.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#81813): https://edk2.groups.io/g/devel/message/81813 Mute This Topic: https://groups.io/mt/86233569/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
