Re: [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock

["Zeng, Star" <star.zeng@intel.com>]

On 2018/10/25 15:20, Wang, Jian J wrote:
> Sorry, forgot to update commit message:
> 
>> This issue is hidden in current code but exposed by introduction
>> of freed-memory guard feature due to the fact that the feature
>> will turn all pool allocation to page allocation.
>>
>> The solution is moving the memory allocation in CoreGetMemorySpaceMap()
>> to be out of the GCD memory map lock.
> 
> Regards,
> Jian
> 
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
>> Sent: Thursday, October 25, 2018 3:18 PM
>> To: edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
>> <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Subject: [edk2] [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire GCD
>> memory lock
>>
>>> v4 changes:
>>> a. add more comments from Laszlo
>>
>> This issue is hidden in current code but exposed by introduction
>> of freed-memory guard feature due to the fact that the feature
>> will turn all pool allocation to page allocation.
>>
>> The solution is move the memory allocation in CoreGetMemorySpaceMap()
>> and CoreGetIoSpaceMap() to be out of the GCD memory map lock.
>>
>>     CoreDumpGcdMemorySpaceMap()
>> => CoreGetMemorySpaceMap()
>> => CoreAcquireGcdMemoryLock () *
>>     AllocatePool()
>> => InternalAllocatePool()
>> => CoreAllocatePool()
>> => CoreAllocatePoolI()
>> => CoreAllocatePoolPagesI()
>> => CoreAllocatePoolPages()
>> => FindFreePages()
>> => PromoteMemoryResource()
>> => CoreAcquireGcdMemoryLock()  **
>>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>> ---
>>   MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 87 +++++++++++++++++++++++++++++-
>> -----------
>>   1 file changed, 62 insertions(+), 25 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
>> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
>> index d9c65a8045..f99d6bb933 100644
>> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
>> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
>> @@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap (
>>     OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR  **MemorySpaceMap
>>     )
>>   {
>> -  EFI_STATUS                       Status;
>>     LIST_ENTRY                       *Link;
>>     EFI_GCD_MAP_ENTRY                *Entry;
>>     EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *Descriptor;
>> +  UINTN                            DescriptorCount;
>>
>>     //
>>     // Make sure parameters are valid
>> @@ -1706,38 +1706,75 @@ CoreGetMemorySpaceMap (
>>       return EFI_INVALID_PARAMETER;
>>     }
>>
>> -  CoreAcquireGcdMemoryLock ();
>> +  *NumberOfDescriptors  = 0;
>> +  *MemorySpaceMap       = NULL;
>>
>>     //
>> -  // Count the number of descriptors
>> +  // Take the lock, for entering the loop with the lock held.
>>     //
>> -  *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
>> +  CoreAcquireGcdMemoryLock ();
>> +  while (TRUE) {
>> +    //
>> +    // Count the number of descriptors. It might be done more than once

How about using "Count the descriptors" here? No need to send new patch 
series for it.

>> because
>> +    // there's code which has to be running outside the GCD lock.

I have given comment for this line at V3 series.
How about using "AllocatePool() calling code below" instead of "there's 
code" to be more specific?

Thanks,
Star

>> +    //
>> +    DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
>> +    if (DescriptorCount == *NumberOfDescriptors) {
>> +      //
>> +      // Fill in the MemorySpaceMap if no memory space map change.
>> +      //
>> +      Descriptor = *MemorySpaceMap;
>> +      Link = mGcdMemorySpaceMap.ForwardLink;
>> +      while (Link != &mGcdMemorySpaceMap) {
>> +        Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
>> +        BuildMemoryDescriptor (Descriptor, Entry);
>> +        Descriptor++;
>> +        Link = Link->ForwardLink;
>> +      }
>> +      //
>> +      // We're done; exit the loop with the lock held.
>> +      //
>> +      break;
>> +    }
>>
>> -  //
>> -  // Allocate the MemorySpaceMap
>> -  //
>> -  *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof
>> (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
>> -  if (*MemorySpaceMap == NULL) {
>> -    Status = EFI_OUT_OF_RESOURCES;
>> -    goto Done;
>> -  }
>> +    //
>> +    // Release the lock before memory allocation, because it might cause
>> +    // GCD lock conflict in one of calling path in AllocatPool().
>> +    //
>> +    CoreReleaseGcdMemoryLock ();
>> +
>> +    //
>> +    // Allocate memory to store the MemorySpaceMap. Note it might be already
>> +    // allocated if there's map descriptor change during memory allocation at
>> +    // last time.
>> +    //
>> +    if (*MemorySpaceMap != NULL) {
>> +      FreePool (*MemorySpaceMap);
>> +    }
>>
>> +    *MemorySpaceMap = AllocatePool (DescriptorCount *
>> +                                    sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
>> +    if (*MemorySpaceMap == NULL) {
>> +      *NumberOfDescriptors = 0;
>> +      return EFI_OUT_OF_RESOURCES;
>> +    }
>> +
>> +    //
>> +    // Save the descriptor count got before for another round of check to make
>> +    // sure we won't miss any, since we have code running outside the GCD lock.
>> +    //
>> +    *NumberOfDescriptors = DescriptorCount;
>> +    //
>> +    // Re-acquire the lock, for the next iteration.
>> +    //
>> +    CoreAcquireGcdMemoryLock ();
>> +  }
>>     //
>> -  // Fill in the MemorySpaceMap
>> +  // We exited the loop with the lock held, release it.
>>     //
>> -  Descriptor = *MemorySpaceMap;
>> -  Link = mGcdMemorySpaceMap.ForwardLink;
>> -  while (Link != &mGcdMemorySpaceMap) {
>> -    Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
>> -    BuildMemoryDescriptor (Descriptor, Entry);
>> -    Descriptor++;
>> -    Link = Link->ForwardLink;
>> -  }
>> -  Status = EFI_SUCCESS;
>> -
>> -Done:
>>     CoreReleaseGcdMemoryLock ();
>> -  return Status;
>> +
>> +  return EFI_SUCCESS;
>>   }
>>
>>
>> --
>> 2.16.2.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel