> On Aug 1, 2018, at 9:52 AM, Jeffrey Hugo <[email protected]> wrote: > > On 8/1/2018 8:56 AM, Eric Snowberg wrote: >> Commit d2078d5adbe2 ("x86: EFI stub support for large memory maps") >> added support for EFI memory maps larger than 128 entries. Following >> allocation for this new buffer (within the exit_boot function calling >> alloc_e820ext) another call to efi_get_memory_map was performed. >> This took place before the final call to ExitBootServices (EBS) to >> insure the memory map buffer was large enough since memory allocation >> is not allowed after calling EBS. >> Later this code was refactored and portions were added to >> efi-stub-helper.c. The call to efi_get_memory_map did not remain after >> calling alloc_e820ext. In a later patch, a new EFI_MMAP_NR_SLACK_SLOTS was >> introduced to help give some headroom should the memory map grow after >> the EBS is called. This does not alway provide enough space with large >> memory maps. > It might be nice if you CC'd the authors of the changes you are "fixing" so > that they can chime in. I guess I'm lucky I noticed this on the list. You > should probably also have a "fixes tag”.
Sorry I missed you. > > Can you elaborate on the specific scenario where EFI_MMAP_NR_SLACK_CLOTS is > insufficient? What is the exact failure encountered? A hard hang. > >> This change will call efi_get_memory_map one final time before calling EBS, >> ensuring there is enough room for the memory map buffer. The additional >> headroom will be left alone for any memory change that may currently >> be in flight between the final efi_get_memory_map call and EBS. > > Uhh, I'm thinking this should be Nack'd. > > This invalidates the entire point of the priv_func because now the memory map > may have changed after it was processed by priv_func, but the mem map may not > have changed between your added call and EBS. If that happens, EBS will > pass, and we'll go into linux with a corrupt mem map. How would we go into Linux with a corrupt memory map? > The mem map can still change between your added call and EBS, so you haven't > really done anything to address the existing race condition. This patch is not trying to fix a race condition. The priv_func is exit_boot_func on x86. exit_boot_func can allocate much more memory than the approx 384 bytes set aside for slack space (thru alloc_e820ext). Once EBS is called, there isn’t enough room available for the new memory map on a very large system (do to the memory that was previously allocated in exit_boot_func). The race condition that exists between priv_func and the call to EBS will be taken care of by the existing code, thru the slack slots. > >> Signed-off-by: Eric Snowberg <[email protected]> >> --- >> drivers/firmware/efi/libstub/efi-stub-helper.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c >> b/drivers/firmware/efi/libstub/efi-stub-helper.c >> index b018436..3363b4e 100644 >> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c >> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c >> @@ -845,6 +845,14 @@ efi_status_t efi_exit_boot_services(efi_system_table_t >> *sys_table_arg, >> if (status != EFI_SUCCESS) >> goto free_map; >> + /* >> + * priv_func could have allocated memory, thus call efi_get_memory_map >> + * one more time. >> + */ >> + status = efi_get_memory_map(sys_table_arg, map); >> + if (status != EFI_SUCCESS) >> + goto fail; >> + >> status = efi_call_early(exit_boot_services, handle, *map->key_ptr); >> if (status == EFI_INVALID_PARAMETER) { > > > -- > Jeffrey Hugo > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, > Inc. > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project. > -- > To unsubscribe from this list: send the line "unsubscribe linux-efi" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
