Re: [PATCH v5 2/2] efi: Free malloc regions on exit

[Daniel Kiper]

On Tue, Aug 29, 2017 at 09:26:48PM +0200, Alexander Graf wrote:
> When we exit grub, we don't free all the memory that we allocated earlier
> for our heap region. This can cause problems with setups where you try
> to descend the boot order using "exit" entries, such as PXE -> HD boot
> scenarios.
>
> Signed-off-by: Alexander Graf <address@hidden>
>
> ---
>
> v2 -> v3:
>
>   - add comment explaining the number of regions
>   - move nr of regions into a define
>   - add warning if we exceed the number of freeable regions
>   - reset region counter to 0 on fini
>
> v3 -> v4:
>
>   - use dynamic list instead of static array at runtime
>   - use allocate_pool for list, so we are not bound by heap or random numbers
>   - remember all allocations, not just the heap
>
> v4 -> v5:
>
>   - free dynamic list entries on allocation removal
> ---
>  grub-core/kern/efi/init.c |  1 +
>  grub-core/kern/efi/mm.c   | 88 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  include/grub/efi/efi.h    |  1 +
>  3 files changed, 90 insertions(+)
>
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 2c31847bf..3dfdf2d22 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -80,4 +80,5 @@ grub_efi_fini (void)
>  {
>    grub_efidisk_fini ();
>    grub_console_fini ();
> +  grub_efi_memory_fini ();
>  }
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index ac2a4c556..c9bd3568d 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -49,6 +49,80 @@ static grub_efi_uintn_t finish_desc_size;
>  static grub_efi_uint32_t finish_desc_version;
>  int grub_efi_is_finished = 0;
>
> +/*
> + * We need to roll back EFI allocations on exit. Remember allocations that
> + * we'll free on exit.
> + */
> +struct efi_allocation;
> +struct efi_allocation {
> +     struct efi_allocation *next;

Please put this member as the last one.

> +     grub_efi_physical_address_t start_addr;

s/start_addr/address/

> +     grub_efi_uint64_t pages;
> +};
> +static struct efi_allocation *efi_allocated_memory;
> +
> +static void
> +grub_efi_unremember_pages (grub_efi_physical_address_t address,
> +                           grub_efi_uintn_t pages)

This function should be after grub_efi_remember_pages().

And maybe s/grub_efi_unremember_pages()/grub_efi_drop_alloc()/...

> +{
> +  struct efi_allocation **allocp;
> +  grub_efi_boot_services_t *b;
> +
> +  b = grub_efi_system_table->boot_services;
> +
> +  for (allocp = &efi_allocated_memory; *allocp;)
> +    {
> +      struct efi_allocation *alloc;
> +      struct efi_allocation *next;
> +
> +      alloc = *allocp;
> +
> +      if (alloc->start_addr != address ||
> +          alloc->pages != pages)
> +        {
> +          /* Move on to the next entry */
> +          allocp = &alloc->next;
> +
> +          continue;
> +        }
> +
> +      /* Remember the next entry */
> +      next = alloc->next;
> +
> +      /* Free the current list entry */
> +      efi_call_1 (b->free_pool, alloc);
> +
> +      /* Remove from list */
> +      *allocp = next;
> +
> +      /* Done */
> +      break;
> +    }

Hmmm... This looks a bit complicated. Could you try this:

struct efi_allocation *ea, *eap;

for (eap = NULL, ea = efi_allocated_memory; ea; eap = ea, ea = ea->next)
  {
    if (ea->start_addr != address || ea->pages != pages)
       continue;

    if (eap)
      efi_allocated_memory = ea->next;
    else
      eap->next = ea->next;

    efi_call_1 (b->free_pool, ea);

    return;
  }

> +}
> +
> +static void
> +grub_efi_remember_pages (grub_efi_physical_address_t address,
> +                         grub_efi_uintn_t pages)

This function should be before grub_efi_unremember_pages().

And maybe s/grub_efi_remember_pages()/grub_efi_store_alloc()/...

> +{
> +  grub_efi_boot_services_t *b;
> +  struct efi_allocation *alloc;
> +  grub_efi_status_t status;
> +
> +  b = grub_efi_system_table->boot_services;
> +  status = efi_call_3 (b->allocate_pool, GRUB_EFI_LOADER_DATA,
> +                           sizeof(*alloc), (void**)&alloc);
> +  if (status == GRUB_EFI_SUCCESS)
> +    {
> +      alloc->next = efi_allocated_memory;
> +      alloc->start_addr = address;
> +      alloc->pages = pages;
> +      efi_allocated_memory = alloc;
> +    }
> +  else
> +      grub_printf ("Could not malloc memory to remember EFI allocation. "
> +                   "Exiting grub2 won't free all memory.\n");

s/grub2/GRUB2/

> +}
> +
>  /* Allocate pages. Return the pointer to the first of allocated pages.  */
>  void *
>  grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
> @@ -79,6 +153,7 @@ grub_efi_allocate_pages_real (grub_efi_physical_address_t 
> address,
>       return 0;
>      }
>
> +  grub_efi_remember_pages (address, pages);
>    return (void *) ((grub_addr_t) address);
>  }
>
> @@ -108,6 +183,7 @@ grub_efi_free_pages (grub_efi_physical_address_t address,
>
>    b = grub_efi_system_table->boot_services;
>    efi_call_2 (b->free_pages, address, pages);
> +  grub_efi_unremember_pages (address, pages);
>  }
>
>  #if defined (__i386__) || defined (__x86_64__)
> @@ -422,6 +498,18 @@ add_memory_regions (grub_efi_memory_descriptor_t 
> *memory_map,
>      grub_fatal ("too little memory");
>  }
>
> +void
> +grub_efi_memory_fini (void)
> +{
> +  /* Free all stale allocations */
> +

Drop this empty line. And please improve the comment here. It took me
some time to understand why just "while (efi_allocated_memory)" works
here. grub_efi_free_pages() calls grub_efi_unremember_pages()
which advances the pointer.

> +  while (efi_allocated_memory)
> +      grub_efi_free_pages (efi_allocated_memory->start_addr,
> +                           efi_allocated_memory->pages);
> +
> +  efi_allocated_memory = NULL;

I have a feeling that you do not need this here.

Daniel