From: "Dandan Bi" <dandan.bi@intel.com>
To: "Ma, Hua" <hua.ma@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>,
Liming Gao <gaoliming@byosoft.com.cn>,
"Ni, Ray" <ray.ni@intel.com>
Subject: Re: [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList
Date: Tue, 12 Oct 2021 04:13:07 +0000 [thread overview]
Message-ID: <DM4PR11MB5453C2AF00A9C51A7EA0A2DCEAB69@DM4PR11MB5453.namprd11.prod.outlook.com> (raw)
In-Reply-To: <7c0683f0a02729d8c75dcb631fec2941c834629e.1633948972.git.hua.ma@intel.com>
Hi Hua,
One minor comment inline, please check.
Thanks,
Dandan
> -----Original Message-----
> From: Ma, Hua <hua.ma@intel.com>
> Sent: Monday, October 11, 2021 6:45 PM
> To: devel@edk2.groups.io
> Cc: Ma, Hua <hua.ma@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Liming Gao <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>;
> Ni, Ray <ray.ni@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.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Hua Ma <hua.ma@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
next prev parent reply other threads:[~2021-10-12 4:13 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-11 10:45 [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList Ma, Hua
2021-10-11 11:28 ` [edk2-devel] " Michael Brown
2021-10-11 14:23 ` Ma, Hua
2021-10-12 4:13 ` Dandan Bi [this message]
2021-10-12 8:37 ` Ma, Hua
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM4PR11MB5453C2AF00A9C51A7EA0A2DCEAB69@DM4PR11MB5453.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox