Re: [PATCH V2 3/4] ofnet: free memory on module unload

[Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>]

On 12/10/2016 09:18 PM, Andrei Borzenkov wrote:
> 02.12.2016 18:10, Stanislav Kholmanskikh пишет:
>> On module unload each of its network cards are unregistered,
>> but corresponding memory areas are not freed.
>>
>> This commit is to fix this situation.
>>
>> Freeing the transmit buffer goes via a special function, since
>> it is allocated via ofnet_alloc_netbuf().
>>
>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
>> ---
>>  grub-core/net/drivers/ieee1275/ofnet.c |   61 +++++++++++++++++++++++++++++++-
>>  1 files changed, 60 insertions(+), 1 deletions(-)
>>
>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
>> index 25559c8..1f8ac9a 100644
>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>> @@ -331,6 +331,40 @@ grub_ieee1275_alloc_mem (grub_size_t len)
>>      return (void *)args.result;
>>  }
>>  
>> +/* Free memory allocated by alloc-mem */
>> +static grub_err_t
>> +grub_ieee1275_free_mem (void *addr, grub_size_t len)
>> +{
>> +  struct free_args
>> +  {
>> +    struct grub_ieee1275_common_hdr common;
>> +    grub_ieee1275_cell_t method;
>> +    grub_ieee1275_cell_t len;
>> +    grub_ieee1275_cell_t addr;
>> +    grub_ieee1275_cell_t catch;
>> +  }
>> +  args;
>> +
>> +  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
>> +    {
>> +      grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
>> +      return grub_errno;
>> +    }
>> +
>> +  INIT_IEEE1275_COMMON (&args.common, "interpret", 3, 1);
>> +  args.addr = (grub_ieee1275_cell_t)addr;
>> +  args.len = len;
>> +  args.method = (grub_ieee1275_cell_t) "free-mem";
>> +
>> +  if (IEEE1275_CALL_ENTRY_FN(&args) == -1 || args.catch)
>> +    {
>> +      grub_error (GRUB_ERR_INVALID_COMMAND, N_("free-mem failed"));
>> +      return grub_errno;
>> +    }
>> +
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>>  static void *
>>  ofnet_alloc_netbuf (grub_size_t len)
>>  {
>> @@ -340,6 +374,15 @@ ofnet_alloc_netbuf (grub_size_t len)
>>      return grub_zalloc (len);
>>  }
>>  
>> +static void
>> +ofnet_free_netbuf (void *addr, grub_size_t len)
>> +{
>> +  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
>> +    grub_ieee1275_free_mem (addr, len);
>> +  else
>> +    grub_free (addr);
>> +}
>> +
>>  static int
>>  search_net_devices (struct grub_ieee1275_devalias *alias)
>>  {
>> @@ -494,9 +537,25 @@ GRUB_MOD_INIT(ofnet)
>>  GRUB_MOD_FINI(ofnet)
>>  {
>>    struct grub_net_card *card, *next;
>> +  struct grub_ofnetcard_data *ofdata;
>>  
>>    FOR_NET_CARDS_SAFE (card, next) 
>>      if (card->driver && grub_strcmp (card->driver->name, "ofnet") == 0)
>> -      grub_net_card_unregister (card);
>> +      {
>> +	grub_net_card_unregister (card);
>> +	/*
>> +	 * The fact that we are here means the card was successfully
>> +	 * initialized in the past, so all the below pointers are valid,
>> +	 * and we may free associated memory without checks.
>> +	 */
>> +	ofdata = (struct grub_ofnetcard_data *) card->data;
>> +	grub_free (ofdata->path);
>> +	grub_free (ofdata);
>> +
>> +	ofnet_free_netbuf (card->txbuf, card->txbufsize);
>> +
>> +	grub_free ((void *) card->name);
>> +	grub_free (card);
>> +      }
> 
> No, it's not safe to do. I plunged into it in efinet and reverted. We
> may have dangling references to card left, so you cannot free it (at
> least, please not at this point before release and not without prior
> audit). See
> 
> commit cc699535e57e0d0f099090e64a63037c7834f104
> Author: Andrei Borzenkov <arvidjaar@gmail.com>
> Date:   Mon May 4 09:13:53 2015 +0300
> 
>     Revert "efinet: memory leak on module removal"
> 

Thanks. This is sad.

I'll exclude these changes from V3 then.


> 
>>    grub_ieee1275_net_config = 0;
>>  }
>>
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>