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 <[email protected]> >> --- >> 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 <[email protected]> > 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 > [email protected] > https://lists.gnu.org/mailman/listinfo/grub-devel > _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
