[V2] Implementing the receive buffer for ofnet

[V2] Implementing the receive buffer for ofnet

[Stanislav Kholmanskikh]

Hi!

This is V2 of the series:

http://lists.gnu.org/archive/html/grub-devel/2016-04/msg00015.html

with the final aim of enabling the receive buffer in the ofnet module.

The first version received a number of comments:

http://lists.gnu.org/archive/html/grub-devel/2016-11/msg00054.html
http://lists.gnu.org/archive/html/grub-devel/2016-11/msg00057.html

and I tried to address them all in this new version.

Summary of changes:

1. The error check for grub_netbuff_reserve() moved into a separate patch.

2. Previously, ofnet_alloc_netbuf() and ofnet_free_netbuf() were
in one patch. I decided to split this patch into two:
 - one is for ofnet_alloc_netbuf()
 - the other is for ofnet_free_netbuf() + freeing memory on module onload

This looks logical.

The (IEEE1275..... || args.catch) expressions were made one-line.

3. Added a patch to free the card memory on module unload

4. In the last patch in get_card_packet() I decided to follow my original
proposal:

+  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;

This, in my opinion, makes the code flow to be more consistent. Each 'if'
in this function checks for an error and its code block is the code
to be executed when the error happens. If everything is fine we reach
the final statement of the function.

5. As for the perfomance impact of the last patch in the series.

On one LDOM I setup a TFTP server and put an initrd image of 1.1G size
there.

A different LDOM was used as a host to boot GRUB and ofnet. This LDOM
had a network interface:

{0} ok cd net0
{0} ok .properties
local-mac-address        00 14 4f f8 a9 dd
max-frame-size           00004000
address-bits             00000030
reg                      00000000
compatible               SUNW,sun4v-network
device_type              network
name                     network
{0} ok

i.e. the MTU is 0x4000.

The two LDOMs were located at one physical host. Both were connected
to one virtual switch.

The test scenario was:
1. Boot grub2 in the latter ldom
2. Execute
 set root=tftp,ip_of_the_former_host
3. Execute
 linux /vmlinuz
   Otherwise the 'initrd' command won't work.

4. Measure the time required for command:
 initrd /initrd.img
   to complete.

This test scenario was executed when the grub2 was compiled with the last
patch in the series and when it was compiled without the patch. 3 iterations
for each case.

In both the cases the aveage time to load the 1.1G initrd was ~68 sec.

I could not perform ths test scenario on my bare-metal machines. Their
network adapters have the MTU of 0x10000 and even booting of core grub2 modules,
like normal.mod, results in a series of:

error: out of memory.
error: out of memory.
error: out of memory.

This is without the receive buffer. With the receive buffer booting goes
fine and loading of initrd also goes fine.

Thanks.

[PATCH V2 1/4] ofnet: add error check for grub_netbuff_reserve

[Stanislav Kholmanskikh]

Signed-off-by: Stanislav Kholmanskikh <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
-- 
1.7.1

[PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function

[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 <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;
-- 
1.7.1

[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 <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;
 }
-- 
1.7.1

[PATCH V2 4/4] ofnet: implement the receive buffer

[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 <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);
-- 
1.7.1

Re: [PATCH V2 1/4] ofnet: add error check for grub_netbuff_reserve

[Daniel Kiper]

On Fri, Dec 02, 2016 at 06:10:02PM +0300, Stanislav Kholmanskikh wrote:
> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>

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

Daniel

Re: [PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function

[Daniel Kiper]

On Fri, Dec 02, 2016 at 06:10:03PM +0300, Stanislav Kholmanskikh wrote:
> 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 <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;

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

Daniel

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

[Daniel Kiper]

On Fri, Dec 02, 2016 at 06:10:04PM +0300, Stanislav Kholmanskikh wrote:
> 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>

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

Daniel

Re: [PATCH V2 4/4] ofnet: implement the receive buffer

[Daniel Kiper]

On Fri, Dec 02, 2016 at 06:10:05PM +0300, Stanislav Kholmanskikh wrote:
> 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 <stanislav.kholmanskikh@oracle.com>

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

Daniel

Re: [PATCH V2 1/4] ofnet: add error check for grub_netbuff_reserve

[Andrei Borzenkov]

02.12.2016 18:10, Stanislav Kholmanskikh пишет:
> Signed-off-by: Stanislav Kholmanskikh <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
> 

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.

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

[Andrei Borzenkov]

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"


>    grub_ieee1275_net_config = 0;
>  }
> 

Re: [PATCH V2 4/4] ofnet: implement the receive buffer

[Andrei Borzenkov]

02.12.2016 18:10, 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 <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);
> 

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

RE: [PATCH V2 4/4] ofnet: implement the receive buffer

[Mark Otto]

Understood.

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

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

02.12.2016 18:10, 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 
> <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);
> 

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

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

Re: [PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function

[Stanislav Kholmanskikh]

On 12/05/2016 06:52 PM, Daniel Kiper wrote:
> On Fri, Dec 02, 2016 at 06:10:03PM +0300, Stanislav Kholmanskikh wrote:
>> 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 <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;
> 
> 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.



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

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

[Stanislav Kholmanskikh]

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
> 

Re: [PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function

[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
<stanislav.kholmanskikh@oracle.com> wrote:
>
>
> On 12/05/2016 06:52 PM, Daniel Kiper wrote:
>> On Fri, Dec 02, 2016 at 06:10:03PM +0300, Stanislav Kholmanskikh wrote:
>>> 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 <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;
>>
>> 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.
>
>
>
>>
>> Daniel
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH V2 1/4] ofnet: add error check for grub_netbuff_reserve

[Stanislav Kholmanskikh]

On 12/10/2016 09:08 PM, Andrei Borzenkov wrote:
> 02.12.2016 18:10, Stanislav Kholmanskikh пишет:
>> Signed-off-by: Stanislav Kholmanskikh <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
>>
> 
> 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.

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

Re: [PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function

[Stanislav Kholmanskikh]

On 12/12/2016 01:27 PM, Andrei Borzenkov wrote:
> 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.

> 
> On Mon, Dec 12, 2016 at 12:47 PM, Stanislav Kholmanskikh
> <stanislav.kholmanskikh@oracle.com> wrote:
>>
>>
>> On 12/05/2016 06:52 PM, Daniel Kiper wrote:
>>> On Fri, Dec 02, 2016 at 06:10:03PM +0300, Stanislav Kholmanskikh wrote:
>>>> 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 <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;
>>>
>>> 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.
>>
>>
>>
>>>
>>> Daniel
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 

Re: [PATCH V2 1/4] ofnet: add error check for grub_netbuff_reserve

[Andrei Borzenkov]

On Mon, Dec 12, 2016 at 1:34 PM, Stanislav Kholmanskikh
<stanislav.kholmanskikh@oracle.com> wrote:
>
>
> On 12/10/2016 09:08 PM, Andrei Borzenkov wrote:
>> 02.12.2016 18:10, Stanislav Kholmanskikh пишет:
>>> Signed-off-by: Stanislav Kholmanskikh <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
>>>
>>
>> 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.

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