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

[Daniel Kiper]

On Thu, Aug 31, 2017 at 02:10:48PM +0200, Alexander Graf wrote:
> On 08/30/2017 02:11 PM, Daniel Kiper wrote:
> >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.
>
> Sure, but care to explain why? Having the list chaining at the beginning
> of the struct is a very natural thing to do in pretty much every other
> project I've worked on.

OK, let's leave it then.

> >>+   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()/...
> Sure...
>
> >
> >>+{
> >>+  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)
>
> I suppose you mean !eap here

Yep, or even better:

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

> >       efi_allocated_memory = ea->next;
> >     else
> >       eap->next = ea->next;
> >
> >     efi_call_1 (b->free_pool, ea);
> >
> >     return;
> >   }
>
> I'm not sure the version above is actually more readable. Do you really
> want me to move to that? It basically does the same thing, just with an

Yes, please.

> explicit branch in between.

...and lower number of pointers...

> >>+}
> >>+
> >>+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()/...
>
> works for me :)
>
> >
> >>+{
> >>+  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.
>
> Does this work for you?
>
> void
> grub_efi_memory_fini (void)
> {
>   /*
>    * Free all stale allocations. grub_efi_free_pages() will remove
>    * the found entry from the list and it will always find the first
>    * list entry (efi_allocated_memory is the list start). Hence we
>    * remove all entries from the list until none is left altogether.
>    */
>   while (efi_allocated_memory)
>       grub_efi_free_pages (efi_allocated_memory->address,
>                            efi_allocated_memory->pages);
> }

I am OK with this.

Daniel