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

[Alexander Graf <agraf@suse.de>]

On 05/24/2016 06:56 AM, Elliott, Robert (Persistent Memory) wrote:
>> -----Original Message-----
>> From: Grub-devel [mailto:grub-devel-bounces+elliott=hpe.com@gnu.org]
>> 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