RE: [PATCH] efi: Free malloc regions on exit

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Grub-devel [mailto:address@hidden
> On Behalf Of Alexander Graf
> Sent: Thursday, May 19, 2016 8:38 AM
> Subject: [PATCH] efi: Free malloc regions on exit
...
> +struct efi_allocation {

If no other file is using this, mark it as static.

> +     grub_uint64_t start_addr;

That should be grub_efi_physical_address_t.

> +     grub_uint64_t pages;

That should be grub_efi_uint64_t (the parameter type
for add_memory_regions) or grub_efi_uintn_t (the parameter
type for grub_efi_allocate_pages and grub_efi_free_pages).

> +} efi_allocated_memory[16];

Why 16 and not some other number?  Consider a #define and making
the comment about 16 in add_memory_regions more generic.

> +unsigned int efi_allocated_memory_idx = 0;

The C language definition guarantees that global variables
are initialized to 0.


> @@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t
> *memory_map,
>                   (void *) ((grub_addr_t) start),
>                   (unsigned) pages);
> 
> +      /* Track up to 16 regions that we allocate from */
> +      if (efi_allocated_memory_idx <
> ARRAY_SIZE(efi_allocated_memory)) {
> +        efi_allocated_memory[efi_allocated_memory_idx].start_addr =
> start;
> +        efi_allocated_memory[efi_allocated_memory_idx].pages =
> pages;
> +        efi_allocated_memory_idx++;
> +      }
> +

Consider printing if the magic number is exceeded, rather than
silently changing the behavior.

>        grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
> 
>        required_pages -= pages;
> @@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t
> *memory_map,
>      grub_fatal ("too little memory");
>  }
> 
> +void
> +grub_efi_memory_fini (void)
> +{
> +  unsigned int i;
> +
> +  for (i = 0; i < efi_allocated_memory_idx; i++) {
> +    grub_efi_free_pages (efi_allocated_memory[i].start_addr,
> +                         efi_allocated_memory[i].pages);
> +  }
> +}

Consider clearing efi_allocated_memory_idx after that, since 
this function cannot be sure it's the last one to use a
global variable.

Clearing start_addr might help prevent stale pointer
reference bugs.  Pointers that are no longer valid are 
dangerous to leave around.


---
Robert Elliott, HPE Persistent Memory