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

[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 <***@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);
+ }
grub_ieee1275_net_config = 0;
}

[Stanislav Kholmanskikh]

In the current code search_net_devices() uses the "alloc-mem" command
from the IEEE1275 User Interface for allocation of the transmit buffer
for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.

I don't have hardware where this flag is set to verify if this
workaround is still needed. However, further changes to ofnet will
require to execute this workaround one more time. Therefore, to
avoid possible duplication of code I'm moving this piece of
code into a function.

Signed-off-by: Stanislav Kholmanskikh <***@oracle.com>
---
grub-core/net/drivers/ieee1275/ofnet.c | 71 ++++++++++++++++++++------------
1 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 8332d34..25559c8 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -298,6 +298,48 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
}
}

+/* Allocate memory with alloc-mem */
+static void *
+grub_ieee1275_alloc_mem (grub_size_t len)
+{
+ struct alloc_args
+ {
+ struct grub_ieee1275_common_hdr common;
+ grub_ieee1275_cell_t method;
+ grub_ieee1275_cell_t len;
+ grub_ieee1275_cell_t catch;
+ grub_ieee1275_cell_t result;
+ }
+ args;
+
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
+ {
+ grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
+ return NULL;
+ }
+
+ INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
+ args.len = len;
+ args.method = (grub_ieee1275_cell_t) "alloc-mem";
+
+ if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch)
+ {
+ grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
+ return NULL;
+ }
+ else
+ return (void *)args.result;
+}
+
+static void *
+ofnet_alloc_netbuf (grub_size_t len)
+{
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
+ return grub_ieee1275_alloc_mem (len);
+ else
+ return grub_zalloc (len);
+}
+
static int
search_net_devices (struct grub_ieee1275_devalias *alias)
{
@@ -414,39 +456,14 @@ search_net_devices (struct grub_ieee1275_devalias *alias)

card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;

- if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
- {
- struct alloc_args
- {
- struct grub_ieee1275_common_hdr common;
- grub_ieee1275_cell_t method;
- grub_ieee1275_cell_t len;
- grub_ieee1275_cell_t catch;
- grub_ieee1275_cell_t result;
- }
- args;
- INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
- args.len = card->txbufsize;
- args.method = (grub_ieee1275_cell_t) "alloc-mem";
-
- if (IEEE1275_CALL_ENTRY_FN (&args) == -1
- || args.catch)
- {
- card->txbuf = 0;
- grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
- }
- else
- card->txbuf = (void *) args.result;
- }
- else
- card->txbuf = grub_zalloc (card->txbufsize);
+ card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
if (!card->txbuf)
{
grub_free (ofdata->path);
grub_free (ofdata);
grub_free (card);
grub_print_error ();
- return 0;
+ return 1;
}
card->driver = NULL;
card->data = ofdata;

[Daniel Kiper]

Post by Stanislav Kholmanskikh
In the current code search_net_devices() uses the "alloc-mem" command
from the IEEE1275 User Interface for allocation of the transmit buffer
for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.
I don't have hardware where this flag is set to verify if this
workaround is still needed. However, further changes to ofnet will
require to execute this workaround one more time. Therefore, to
avoid possible duplication of code I'm moving this piece of
code into a function.
---
grub-core/net/drivers/ieee1275/ofnet.c | 71 ++++++++++++++++++++------------
1 files changed, 44 insertions(+), 27 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 8332d34..25559c8 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -298,6 +298,48 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
}
}
+/* Allocate memory with alloc-mem */
+static void *
+grub_ieee1275_alloc_mem (grub_size_t len)
+{
+ struct alloc_args
+ {
+ struct grub_ieee1275_common_hdr common;
+ grub_ieee1275_cell_t method;
+ grub_ieee1275_cell_t len;
+ grub_ieee1275_cell_t catch;
+ grub_ieee1275_cell_t result;
+ }
+ args;
+
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
+ {
+ grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
+ return NULL;
+ }
+
+ INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
+ args.len = len;
+ args.method = (grub_ieee1275_cell_t) "alloc-mem";
+
+ if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch)
+ {
+ grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
+ return NULL;
+ }
+ else
+ return (void *)args.result;
+}
+
+static void *
+ofnet_alloc_netbuf (grub_size_t len)
+{
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
+ return grub_ieee1275_alloc_mem (len);
+ else
+ return grub_zalloc (len);
+}
+
static int
search_net_devices (struct grub_ieee1275_devalias *alias)
{
@@ -414,39 +456,14 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
- if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
- {
- struct alloc_args
- {
- struct grub_ieee1275_common_hdr common;
- grub_ieee1275_cell_t method;
- grub_ieee1275_cell_t len;
- grub_ieee1275_cell_t catch;
- grub_ieee1275_cell_t result;
- }
- args;
- INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
- args.len = card->txbufsize;
- args.method = (grub_ieee1275_cell_t) "alloc-mem";
-
- if (IEEE1275_CALL_ENTRY_FN (&args) == -1
- || args.catch)
- {
- card->txbuf = 0;
- grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
- }
- else
- card->txbuf = (void *) args.result;
- }
- else
- card->txbuf = grub_zalloc (card->txbufsize);
+ card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
if (!card->txbuf)
{
grub_free (ofdata->path);
grub_free (ofdata);
grub_free (card);
grub_print_error ();
- return 0;
+ return 1;

Hmmm... This looks like an error...

Daniel

[Stanislav Kholmanskikh]

Post by Daniel Kiper

Post by Stanislav Kholmanskikh
In the current code search_net_devices() uses the "alloc-mem" command
from the IEEE1275 User Interface for allocation of the transmit buffer
for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.
I don't have hardware where this flag is set to verify if this
workaround is still needed. However, further changes to ofnet will
require to execute this workaround one more time. Therefore, to
avoid possible duplication of code I'm moving this piece of
code into a function.
---
grub-core/net/drivers/ieee1275/ofnet.c | 71 ++++++++++++++++++++------------
1 files changed, 44 insertions(+), 27 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 8332d34..25559c8 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -298,6 +298,48 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
}
}
+/* Allocate memory with alloc-mem */
+static void *
+grub_ieee1275_alloc_mem (grub_size_t len)
+{
+ struct alloc_args
+ {
+ struct grub_ieee1275_common_hdr common;
+ grub_ieee1275_cell_t method;
+ grub_ieee1275_cell_t len;
+ grub_ieee1275_cell_t catch;
+ grub_ieee1275_cell_t result;
+ }
+ args;
+
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
+ {
+ grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
+ return NULL;
+ }
+
+ INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
+ args.len = len;
+ args.method = (grub_ieee1275_cell_t) "alloc-mem";
+
+ if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch)
+ {
+ grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
+ return NULL;
+ }
+ else
+ return (void *)args.result;
+}
+
+static void *
+ofnet_alloc_netbuf (grub_size_t len)
+{
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
+ return grub_ieee1275_alloc_mem (len);
+ else
+ return grub_zalloc (len);
+}
+
static int
search_net_devices (struct grub_ieee1275_devalias *alias)
{
@@ -414,39 +456,14 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
- if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
- {
- struct alloc_args
- {
- struct grub_ieee1275_common_hdr common;
- grub_ieee1275_cell_t method;
- grub_ieee1275_cell_t len;
- grub_ieee1275_cell_t catch;
- grub_ieee1275_cell_t result;
- }
- args;
- INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
- args.len = card->txbufsize;
- args.method = (grub_ieee1275_cell_t) "alloc-mem";
-
- if (IEEE1275_CALL_ENTRY_FN (&args) == -1
- || args.catch)
- {
- card->txbuf = 0;
- grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
- }
- else
- card->txbuf = (void *) args.result;
- }
- else
- card->txbuf = grub_zalloc (card->txbufsize);
+ card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
if (!card->txbuf)
{
grub_free (ofdata->path);
grub_free (ofdata);
grub_free (card);
grub_print_error ();
- return 0;
+ return 1;

Hmmm... This looks like an error...

Look, here is what I see.

The search_net_devices() function is called from grub_ofnet_findcards() as:

grub_ieee1275_devices_iterate (search_net_devices);

The grub_ieee1275_devices_iterate(hook) function is defined in
grub-core/kern/ieee1275/openfw.c and what it does is basically calling
the hook for each IEEE1275 device in the tree until:
a) there are no more devices
b) the hook returns a value != 1

So if search_net_devices() returns 1 it means that further probing for
network cards is stopped.

I think that stopping further probes when a memory allocation function
fails is fine and it aligns with the existing code at the top of the
function, i.e. handling of the cases when allocating memory for 'card'
and 'ofdata' fails.

If I'm not mistaken, we may also need to update the block:

if (need_suffix)
ofdata->path = grub_malloc (grub_strlen (alias->path) + sizeof
(SUFFIX));
else
ofdata->path = grub_malloc (grub_strlen (alias->path) + 1);
if (!ofdata->path)
{
grub_print_error ();
return 0;
}

and add a 'return 1' + free some memory there.

As for the other block:

pprop = (grub_uint8_t *) &prop;
if (grub_ieee1275_get_property (devhandle, "mac-address",
pprop, sizeof(prop), &prop_size)
&& grub_ieee1275_get_property (devhandle, "local-mac-address",
pprop, sizeof(prop), &prop_size))
{
grub_error (GRUB_ERR_IO, "Couldn't retrieve mac address.");
grub_print_error ();
return 0;
}

it seems we need to add free memory procedures here as well, but I'm not
sure we need to return 1 here.

Post by Daniel Kiper
Daniel
_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel

[Andrei Borzenkov]

While I agree with your reasons, it belongs to separate commit, and
definitely is out of place if you say in commit message "I'm moving
piece of code". Actually, it is not related to this patch series, so
feel free to send this cleanup as separate patch.

On Mon, Dec 12, 2016 at 12:47 PM, Stanislav Kholmanskikh

Post by Stanislav Kholmanskikh

Post by Daniel Kiper

Post by Stanislav Kholmanskikh
In the current code search_net_devices() uses the "alloc-mem" command
from the IEEE1275 User Interface for allocation of the transmit buffer
for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.
I don't have hardware where this flag is set to verify if this
workaround is still needed. However, further changes to ofnet will
require to execute this workaround one more time. Therefore, to
avoid possible duplication of code I'm moving this piece of
code into a function.
---
grub-core/net/drivers/ieee1275/ofnet.c | 71 ++++++++++++++++++++------------
1 files changed, 44 insertions(+), 27 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 8332d34..25559c8 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -298,6 +298,48 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
}
}
+/* Allocate memory with alloc-mem */
+static void *
+grub_ieee1275_alloc_mem (grub_size_t len)
+{
+ struct alloc_args
+ {
+ struct grub_ieee1275_common_hdr common;
+ grub_ieee1275_cell_t method;
+ grub_ieee1275_cell_t len;
+ grub_ieee1275_cell_t catch;
+ grub_ieee1275_cell_t result;
+ }
+ args;
+
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
+ {
+ grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
+ return NULL;
+ }
+
+ INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
+ args.len = len;
+ args.method = (grub_ieee1275_cell_t) "alloc-mem";
+
+ if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch)
+ {
+ grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
+ return NULL;
+ }
+ else
+ return (void *)args.result;
+}
+
+static void *
+ofnet_alloc_netbuf (grub_size_t len)
+{
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
+ return grub_ieee1275_alloc_mem (len);
+ else
+ return grub_zalloc (len);
+}
+
static int
search_net_devices (struct grub_ieee1275_devalias *alias)
{
@@ -414,39 +456,14 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
- if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
- {
- struct alloc_args
- {
- struct grub_ieee1275_common_hdr common;
- grub_ieee1275_cell_t method;
- grub_ieee1275_cell_t len;
- grub_ieee1275_cell_t catch;
- grub_ieee1275_cell_t result;
- }
- args;
- INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
- args.len = card->txbufsize;
- args.method = (grub_ieee1275_cell_t) "alloc-mem";
-
- if (IEEE1275_CALL_ENTRY_FN (&args) == -1
- || args.catch)
- {
- card->txbuf = 0;
- grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
- }
- else
- card->txbuf = (void *) args.result;
- }
- else
- card->txbuf = grub_zalloc (card->txbufsize);
+ card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
if (!card->txbuf)
{
grub_free (ofdata->path);
grub_free (ofdata);
grub_free (card);
grub_print_error ();
- return 0;
+ return 1;

Hmmm... This looks like an error...

Look, here is what I see.
grub_ieee1275_devices_iterate (search_net_devices);
The grub_ieee1275_devices_iterate(hook) function is defined in
grub-core/kern/ieee1275/openfw.c and what it does is basically calling
a) there are no more devices
b) the hook returns a value != 1
So if search_net_devices() returns 1 it means that further probing for
network cards is stopped.
I think that stopping further probes when a memory allocation function
fails is fine and it aligns with the existing code at the top of the
function, i.e. handling of the cases when allocating memory for 'card'
and 'ofdata' fails.
if (need_suffix)
ofdata->path = grub_malloc (grub_strlen (alias->path) + sizeof
(SUFFIX));
else
ofdata->path = grub_malloc (grub_strlen (alias->path) + 1);
if (!ofdata->path)
{
grub_print_error ();
return 0;
}
and add a 'return 1' + free some memory there.
pprop = (grub_uint8_t *) &prop;
if (grub_ieee1275_get_property (devhandle, "mac-address",
pprop, sizeof(prop), &prop_size)
&& grub_ieee1275_get_property (devhandle, "local-mac-address",
pprop, sizeof(prop), &prop_size))
{
grub_error (GRUB_ERR_IO, "Couldn't retrieve mac address.");
grub_print_error ();
return 0;
}
it seems we need to add free memory procedures here as well, but I'm not
sure we need to return 1 here.

Post by Daniel Kiper
Daniel
_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel

_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel

[Stanislav Kholmanskikh]

Post by Andrei Borzenkov
While I agree with your reasons, it belongs to separate commit, and
definitely is out of place if you say in commit message "I'm moving
piece of code". Actually, it is not related to this patch series, so
feel free to send this cleanup as separate patch.

Thank you. I'll stick to 'return 0' for this series.

Post by Andrei Borzenkov
On Mon, Dec 12, 2016 at 12:47 PM, Stanislav Kholmanskikh

Post by Stanislav Kholmanskikh

Post by Daniel Kiper

Post by Stanislav Kholmanskikh
In the current code search_net_devices() uses the "alloc-mem" command
from the IEEE1275 User Interface for allocation of the transmit buffer
for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.
I don't have hardware where this flag is set to verify if this
workaround is still needed. However, further changes to ofnet will
require to execute this workaround one more time. Therefore, to
avoid possible duplication of code I'm moving this piece of
code into a function.
---
grub-core/net/drivers/ieee1275/ofnet.c | 71 ++++++++++++++++++++------------
1 files changed, 44 insertions(+), 27 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 8332d34..25559c8 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -298,6 +298,48 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
}
}
+/* Allocate memory with alloc-mem */
+static void *
+grub_ieee1275_alloc_mem (grub_size_t len)
+{
+ struct alloc_args
+ {
+ struct grub_ieee1275_common_hdr common;
+ grub_ieee1275_cell_t method;
+ grub_ieee1275_cell_t len;
+ grub_ieee1275_cell_t catch;
+ grub_ieee1275_cell_t result;
+ }
+ args;
+
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
+ {
+ grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
+ return NULL;
+ }
+
+ INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
+ args.len = len;
+ args.method = (grub_ieee1275_cell_t) "alloc-mem";
+
+ if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch)
+ {
+ grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
+ return NULL;
+ }
+ else
+ return (void *)args.result;
+}
+
+static void *
+ofnet_alloc_netbuf (grub_size_t len)
+{
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
+ return grub_ieee1275_alloc_mem (len);
+ else
+ return grub_zalloc (len);
+}
+
static int
search_net_devices (struct grub_ieee1275_devalias *alias)
{
@@ -414,39 +456,14 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
- if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
- {
- struct alloc_args
- {
- struct grub_ieee1275_common_hdr common;
- grub_ieee1275_cell_t method;
- grub_ieee1275_cell_t len;
- grub_ieee1275_cell_t catch;
- grub_ieee1275_cell_t result;
- }
- args;
- INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
- args.len = card->txbufsize;
- args.method = (grub_ieee1275_cell_t) "alloc-mem";
-
- if (IEEE1275_CALL_ENTRY_FN (&args) == -1
- || args.catch)
- {
- card->txbuf = 0;
- grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
- }
- else
- card->txbuf = (void *) args.result;
- }
- else
- card->txbuf = grub_zalloc (card->txbufsize);
+ card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
if (!card->txbuf)
{
grub_free (ofdata->path);
grub_free (ofdata);
grub_free (card);
grub_print_error ();
- return 0;
+ return 1;

Hmmm... This looks like an error...

Look, here is what I see.
grub_ieee1275_devices_iterate (search_net_devices);
The grub_ieee1275_devices_iterate(hook) function is defined in
grub-core/kern/ieee1275/openfw.c and what it does is basically calling
a) there are no more devices
b) the hook returns a value != 1
So if search_net_devices() returns 1 it means that further probing for
network cards is stopped.
I think that stopping further probes when a memory allocation function
fails is fine and it aligns with the existing code at the top of the
function, i.e. handling of the cases when allocating memory for 'card'
and 'ofdata' fails.
if (need_suffix)
ofdata->path = grub_malloc (grub_strlen (alias->path) + sizeof
(SUFFIX));
else
ofdata->path = grub_malloc (grub_strlen (alias->path) + 1);
if (!ofdata->path)
{
grub_print_error ();
return 0;
}
and add a 'return 1' + free some memory there.
pprop = (grub_uint8_t *) &prop;
if (grub_ieee1275_get_property (devhandle, "mac-address",
pprop, sizeof(prop), &prop_size)
&& grub_ieee1275_get_property (devhandle, "local-mac-address",
pprop, sizeof(prop), &prop_size))
{
grub_error (GRUB_ERR_IO, "Couldn't retrieve mac address.");
grub_print_error ();
return 0;
}
it seems we need to add free memory procedures here as well, but I'm not
sure we need to return 1 here.

Post by Daniel Kiper
Daniel
_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel

_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel

_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel

[Stanislav Kholmanskikh]

Signed-off-by: Stanislav Kholmanskikh <***@oracle.com>
---
grub-core/net/drivers/ieee1275/ofnet.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 6bd3b92..8332d34 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -90,7 +90,11 @@ get_card_packet (struct grub_net_card *dev)
return NULL;
/* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
by 4. So that IP header is aligned on 4 bytes. */
- grub_netbuff_reserve (nb, 2);
+ if (grub_netbuff_reserve (nb, 2))
+ {
+ grub_netbuff_free (nb);
+ return NULL;
+ }

start_time = grub_get_time_ms ();
do

[Daniel Kiper]

Could you explain in a few words why it is needed?

Daniel

[Andrei Borzenkov]

Post by Stanislav Kholmanskikh
---
grub-core/net/drivers/ieee1275/ofnet.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 6bd3b92..8332d34 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -90,7 +90,11 @@ get_card_packet (struct grub_net_card *dev)
return NULL;
/* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
by 4. So that IP header is aligned on 4 bytes. */
- grub_netbuff_reserve (nb, 2);
+ if (grub_netbuff_reserve (nb, 2))
+ {
+ grub_netbuff_free (nb);
+ return NULL;
+ }
start_time = grub_get_time_ms ();
do

We already account for this reserve when we allocate netbuf. So this is
redundant. May be short comment before grub_netbuf_alloc to explain how
we get at final size.

[Stanislav Kholmanskikh]

Post by Andrei Borzenkov

Post by Stanislav Kholmanskikh
---
grub-core/net/drivers/ieee1275/ofnet.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 6bd3b92..8332d34 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -90,7 +90,11 @@ get_card_packet (struct grub_net_card *dev)
return NULL;
/* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
by 4. So that IP header is aligned on 4 bytes. */
- grub_netbuff_reserve (nb, 2);
+ if (grub_netbuff_reserve (nb, 2))
+ {
+ grub_netbuff_free (nb);
+ return NULL;
+ }
start_time = grub_get_time_ms ();
do

We already account for this reserve when we allocate netbuf. So this is
redundant. May be short comment before grub_netbuf_alloc to explain how
we get at final size.

So your message is that since nb = grub_netbuff_alloc(some_len + 2)
succeeds (i.e. returns a valid buffer), then following
grub_netbuff_reserve(nb, 2) will never fail and needs no check for
error. Right?

Personally I don't like skipping such checks, but if you insist on
removing the check, then, I think, all other drivers should be updated
as well, because they all have the check.

Thanks.

Post by Andrei Borzenkov
_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel

[Andrei Borzenkov]

On Mon, Dec 12, 2016 at 1:34 PM, Stanislav Kholmanskikh

Post by Stanislav Kholmanskikh

Post by Andrei Borzenkov

Post by Stanislav Kholmanskikh
---
grub-core/net/drivers/ieee1275/ofnet.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 6bd3b92..8332d34 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -90,7 +90,11 @@ get_card_packet (struct grub_net_card *dev)
return NULL;
/* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
by 4. So that IP header is aligned on 4 bytes. */
- grub_netbuff_reserve (nb, 2);
+ if (grub_netbuff_reserve (nb, 2))
+ {
+ grub_netbuff_free (nb);
+ return NULL;
+ }
start_time = grub_get_time_ms ();
do

We already account for this reserve when we allocate netbuf. So this is
redundant. May be short comment before grub_netbuf_alloc to explain how
we get at final size.

So your message is that since nb = grub_netbuff_alloc(some_len + 2)
succeeds (i.e. returns a valid buffer), then following
grub_netbuff_reserve(nb, 2) will never fail and needs no check for
error. Right?

In this place - yes.

Post by Stanislav Kholmanskikh
Personally I don't like skipping such checks, but if you insist on
removing the check, then, I think, all other drivers should be updated
as well, because they all have the check.

The code is rather inconsistent, I agree, but this is not bug fix that
is required as part of your patch series, so any cleanup belongs
elsewhere.

[Stanislav Kholmanskikh]

get_card_packet() from ofnet.c allocates a netbuff based on the device's MTU:

nb = grub_netbuff_alloc (dev->mtu + 64 + 2);

In the case when the MTU is large, and the received packet is
relatively small, this leads to allocation of significantly more memory,
than it's required. An example could be transmission of TFTP packets
with 0x400 blksize via a network card with 0x10000 MTU.

This patch implements a per-card receive buffer in a way similar to efinet.c,
and makes get_card_packet() allocate a netbuff of the received data size.

Signed-off-by: Stanislav Kholmanskikh <***@oracle.com>
---
grub-core/net/drivers/ieee1275/ofnet.c | 50 ++++++++++++++++++++++---------
1 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 1f8ac9a..471b87b 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -85,9 +85,18 @@ get_card_packet (struct grub_net_card *dev)
grub_uint64_t start_time;
struct grub_net_buff *nb;

- nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
+ start_time = grub_get_time_ms ();
+ do
+ rc = grub_ieee1275_read (data->handle, dev->rcvbuf, dev->rcvbufsize, &actual);
+ while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200));
+
+ if (actual <= 0)
+ return NULL;
+
+ nb = grub_netbuff_alloc (actual + 2);
if (!nb)
return NULL;
+
/* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
by 4. So that IP header is aligned on 4 bytes. */
if (grub_netbuff_reserve (nb, 2))
@@ -96,17 +105,15 @@ get_card_packet (struct grub_net_card *dev)
return NULL;
}

- start_time = grub_get_time_ms ();
- do
- rc = grub_ieee1275_read (data->handle, nb->data, dev->mtu + 64, &actual);
- while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200));
- if (actual > 0)
+ grub_memcpy (nb->data, dev->rcvbuf, actual);
+
+ if (grub_netbuff_put (nb, actual))
{
- grub_netbuff_put (nb, actual);
- return nb;
+ grub_netbuff_free (nb);
+ return NULL;
}
- grub_netbuff_free (nb);
- return NULL;
+
+ return nb;
}

static struct grub_net_card_driver ofdriver =
@@ -498,16 +505,21 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->default_address = lla;

card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
+ card->rcvbufsize = ALIGN_UP (card->mtu, 64) + 256;

card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
if (!card->txbuf)
+ goto fail_netbuf;
+
+ card->rcvbuf = ofnet_alloc_netbuf (card->rcvbufsize);
+ if (!card->rcvbuf)
{
- grub_free (ofdata->path);
- grub_free (ofdata);
- grub_free (card);
- grub_print_error ();
- return 1;
+ grub_error_push ();
+ ofnet_free_netbuf(card->txbuf, card->txbufsize);
+ grub_error_pop ();
+ goto fail_netbuf;
}
+
card->driver = NULL;
card->data = ofdata;
card->flags = 0;
@@ -519,6 +531,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->driver = &ofdriver;
grub_net_card_register (card);
return 0;
+
+fail_netbuf:
+ grub_free (ofdata->path);
+ grub_free (ofdata);
+ grub_free (card);
+ grub_print_error ();
+ return 1;
}

static void
@@ -553,6 +572,7 @@ GRUB_MOD_FINI(ofnet)
grub_free (ofdata);

ofnet_free_netbuf (card->txbuf, card->txbufsize);
+ ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize);

grub_free ((void *) card->name);
grub_free (card);

[Daniel Kiper]

Post by Stanislav Kholmanskikh
nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
In the case when the MTU is large, and the received packet is
relatively small, this leads to allocation of significantly more memory,
than it's required. An example could be transmission of TFTP packets
with 0x400 blksize via a network card with 0x10000 MTU.
This patch implements a per-card receive buffer in a way similar to efinet.c,
and makes get_card_packet() allocate a netbuff of the received data size.

Reviewed-by: Daniel Kiper <***@oracle.com>

Daniel

[Andrei Borzenkov]

Post by Stanislav Kholmanskikh
nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
In the case when the MTU is large, and the received packet is
relatively small, this leads to allocation of significantly more memory,
than it's required. An example could be transmission of TFTP packets
with 0x400 blksize via a network card with 0x10000 MTU.
This patch implements a per-card receive buffer in a way similar to efinet.c,
and makes get_card_packet() allocate a netbuff of the received data size.
---
grub-core/net/drivers/ieee1275/ofnet.c | 50 ++++++++++++++++++++++---------
1 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 1f8ac9a..471b87b 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -85,9 +85,18 @@ get_card_packet (struct grub_net_card *dev)
grub_uint64_t start_time;
struct grub_net_buff *nb;
- nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
+ start_time = grub_get_time_ms ();
+ do
+ rc = grub_ieee1275_read (data->handle, dev->rcvbuf, dev->rcvbufsize, &actual);
+ while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200));
+
+ if (actual <= 0)
+ return NULL;
+
+ nb = grub_netbuff_alloc (actual + 2);
if (!nb)
return NULL;
+
/* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
by 4. So that IP header is aligned on 4 bytes. */
if (grub_netbuff_reserve (nb, 2))
@@ -96,17 +105,15 @@ get_card_packet (struct grub_net_card *dev)
return NULL;
}
- start_time = grub_get_time_ms ();
- do
- rc = grub_ieee1275_read (data->handle, nb->data, dev->mtu + 64, &actual);
- while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200));
- if (actual > 0)
+ grub_memcpy (nb->data, dev->rcvbuf, actual);
+
+ if (grub_netbuff_put (nb, actual))
{
- grub_netbuff_put (nb, actual);
- return nb;
+ grub_netbuff_free (nb);
+ return NULL;
}
- grub_netbuff_free (nb);
- return NULL;
+
+ return nb;
}
static struct grub_net_card_driver ofdriver =
@@ -498,16 +505,21 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->default_address = lla;
card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
+ card->rcvbufsize = ALIGN_UP (card->mtu, 64) + 256;
card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
if (!card->txbuf)
+ goto fail_netbuf;
+
+ card->rcvbuf = ofnet_alloc_netbuf (card->rcvbufsize);
+ if (!card->rcvbuf)
{
- grub_free (ofdata->path);
- grub_free (ofdata);
- grub_free (card);
- grub_print_error ();
- return 1;
+ grub_error_push ();
+ ofnet_free_netbuf(card->txbuf, card->txbufsize);
+ grub_error_pop ();
+ goto fail_netbuf;
}
+
card->driver = NULL;
card->data = ofdata;
card->flags = 0;
@@ -519,6 +531,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->driver = &ofdriver;
grub_net_card_register (card);
return 0;
+
+ grub_free (ofdata->path);
+ grub_free (ofdata);
+ grub_free (card);
+ grub_print_error ();
+ return 1;
}
static void
@@ -553,6 +572,7 @@ GRUB_MOD_FINI(ofnet)
grub_free (ofdata);
ofnet_free_netbuf (card->txbuf, card->txbufsize);
+ ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize);
grub_free ((void *) card->name);
grub_free (card);

See comment to 3/4. Otherwise OK from my side.

[Mark Otto]

Understood.

Mark Otto
Managing Principal, Services
P: 410.290.1411 x161
***@tresys.com | tresys.com

-----Original Message-----
From: Grub-devel [mailto:grub-devel-bounces+motto=***@gnu.org] On Behalf Of Andrei Borzenkov
Sent: Saturday, December 10, 2016 1:29 PM
To: The development of GNU GRUB <grub-***@gnu.org>
Cc: ***@oracle.com
Subject: Re: [PATCH V2 4/4] ofnet: implement the receive buffer

Post by Stanislav Kholmanskikh
nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
In the case when the MTU is large, and the received packet is
relatively small, this leads to allocation of significantly more
memory, than it's required. An example could be transmission of TFTP
packets with 0x400 blksize via a network card with 0x10000 MTU.
This patch implements a per-card receive buffer in a way similar to
efinet.c, and makes get_card_packet() allocate a netbuff of the received data size.
Signed-off-by: Stanislav Kholmanskikh
---
grub-core/net/drivers/ieee1275/ofnet.c | 50 ++++++++++++++++++++++---------
1 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c
b/grub-core/net/drivers/ieee1275/ofnet.c
index 1f8ac9a..471b87b 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -85,9 +85,18 @@ get_card_packet (struct grub_net_card *dev)
grub_uint64_t start_time;
struct grub_net_buff *nb;
- nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
+ start_time = grub_get_time_ms ();
+ do
+ rc = grub_ieee1275_read (data->handle, dev->rcvbuf,
+ dev->rcvbufsize, &actual); while ((actual <= 0 || rc < 0) &&
+ (grub_get_time_ms () - start_time < 200));
+
+ if (actual <= 0)
+ return NULL;
+
+ nb = grub_netbuff_alloc (actual + 2);
if (!nb)
return NULL;
+
/* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
by 4. So that IP header is aligned on 4 bytes. */
if (grub_netbuff_reserve (nb, 2))
@@ -96,17 +105,15 @@ get_card_packet (struct grub_net_card *dev)
return NULL;
}
- start_time = grub_get_time_ms ();
- do
- rc = grub_ieee1275_read (data->handle, nb->data, dev->mtu + 64, &actual);
- while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200));
- if (actual > 0)
+ grub_memcpy (nb->data, dev->rcvbuf, actual);
+
+ if (grub_netbuff_put (nb, actual))
{
- grub_netbuff_put (nb, actual);
- return nb;
+ grub_netbuff_free (nb);
+ return NULL;
}
- grub_netbuff_free (nb);
- return NULL;
+
+ return nb;
}
search_net_devices (struct grub_ieee1275_devalias *alias)
card->default_address = lla;
card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
+ card->rcvbufsize = ALIGN_UP (card->mtu, 64) + 256;
card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
if (!card->txbuf)
+ goto fail_netbuf;
+
+ card->rcvbuf = ofnet_alloc_netbuf (card->rcvbufsize); if
+ (!card->rcvbuf)
{
- grub_free (ofdata->path);
- grub_free (ofdata);
- grub_free (card);
- grub_print_error ();
- return 1;
+ grub_error_push ();
+ ofnet_free_netbuf(card->txbuf, card->txbufsize);
+ grub_error_pop ();
+ goto fail_netbuf;
}
+
card->driver = NULL;
card->data = ofdata;
card->flags = 0;
@@ -519,6 +531,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
card->driver = &ofdriver;
grub_net_card_register (card);
return 0;
+
+ grub_free (ofdata->path);
+ grub_free (ofdata);
+ grub_free (card);
+ grub_print_error ();
+ return 1;
}
static void
@@ -553,6 +572,7 @@ GRUB_MOD_FINI(ofnet)
grub_free (ofdata);
ofnet_free_netbuf (card->txbuf, card->txbufsize);
+ ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize);
grub_free ((void *) card->name);
grub_free (card);

See comment to 3/4. Otherwise OK from my side.

_______________________________________________
Grub-devel mailing list
Grub-***@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[Daniel Kiper]

Post by 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().

Reviewed-by: Daniel Kiper <***@oracle.com>

Daniel

[Andrei Borzenkov]

Post by 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().
---
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 <***@gmail.com>
Date: Mon May 4 09:13:53 2015 +0300

Revert "efinet: memory leak on module removal"

Post by Stanislav Kholmanskikh
grub_ieee1275_net_config = 0;
}

[Stanislav Kholmanskikh]

Post by Andrei Borzenkov

Post by 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().
---
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
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.

Post by Andrei Borzenkov

Post by Stanislav Kholmanskikh
grub_ieee1275_net_config = 0;
}

_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel