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

[Alexander Graf]

On 05/24/2016 06:56 AM, Elliott, Robert (Persistent Memory) wrote:
> > -----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.

Good idea.

> > +       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).

Yup for the type changes.

> > +} 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.

I've added some helpful comment above and moved it into a #define.

> > +unsigned int efi_allocated_memory_idx = 0;
> The C language definition guarantees that global variables
> are initialized to 0.

Yes, but multiple other global variables in the file also initialize 
with 0. Last time I checked gcc just puts those variables into bss 
either way, so you don't really lose anything by explicitly saying = 0 
here. It does increase readability imho though to have it.

> > @@ -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.

Good idea. I don't think anyone really cares enough - there should still 
be enough ram getting free'd on exit if memory is that heavily 
segmented. But I suppose firmware authors want to be aware that their 
memory map is a mess, and this could tell them :).

> >         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.

I'm not sure it's incredibly helpful (since this is a _fini function 
that basically denotes the end of grub's life), but sure.

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

That one however I don't agree with. Array elements are only really 
valid if their counter considers them as valid. If we initialize the 
counter to 0 that means the whole array should automatically be 
considered unused.

Thanks a lot for the review :).


Alex