[PATCH 1/3] efinet: handle get_status() properly

[PATCH 1/3] efinet: handle get_status() properly

[Josef Bacik]

The EFI SNP documentation isn't super clear on the value that is returned in
txbuf when calling into GetStatus.  The documentation says its the pointer to
the recycle buffer, but the documentation for Transmit() says that it should be
the pointer to the buffer that we transmitted.  On the boxes I'm using it's just
a random pointer (usually 0x1).  It is definitely transmitting stuff, but the
get_status call is not returning the pointer to the txbuf we passed in.  Looking
at a few EFI implementations and other SNP drivers it seems like there is
confusion everywhere on this.  So since we only transmit one buffer at a time,
just assume that a non-NULL txbuf means that our transmit happened properly.
With this patch I can now do networking on our EFI enabled boxes.  Thanks,

cc: Peter Jones <pjones@redhat.com>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 grub-core/net/drivers/efi/efinet.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index f27a117..4d3f8aa 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -47,19 +47,11 @@ send_card_buffer (struct grub_net_card *dev,
 	if (st != GRUB_EFI_SUCCESS)
 	  return grub_error (GRUB_ERR_IO,
 			     N_("couldn't send network packet"));
-	if (txbuf == dev->txbuf)
+	if (txbuf)
 	  {
 	    dev->txbusy = 0;
 	    break;
 	  }
-	if (txbuf)
-	  {
-	    st = efi_call_7 (net->transmit, net, 0, dev->last_pkt_size,
-			     dev->txbuf, NULL, NULL, NULL);
-	    if (st != GRUB_EFI_SUCCESS)
-	      return grub_error (GRUB_ERR_IO,
-				 N_("couldn't send network packet"));
-	  }
 	if (limit_time < grub_get_time_ms ())
 	  return grub_error (GRUB_ERR_TIMEOUT,
 			     N_("couldn't send network packet"));
@@ -84,8 +76,9 @@ send_card_buffer (struct grub_net_card *dev,
      we run in the GRUB_ERR_TIMEOUT case above.
      Perhaps a timeout in the FW has discarded the recycle buffer.
    */
+  txbuf = NULL;
   st = efi_call_3 (net->get_status, net, 0, &txbuf);
-  dev->txbusy = !(st == GRUB_EFI_SUCCESS && txbuf == dev->txbuf);
+  dev->txbusy = !(st == GRUB_EFI_SUCCESS && txbuf);
 
   return GRUB_ERR_NONE;
 }
-- 
2.1.0

[PATCH 2/3] net: add local route when creating link local ipv6 interface

[Josef Bacik]

In order to talk to link local ipv6 addresses we need to have a route out of the
link local interface for this to work.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 grub-core/net/net.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 21a4e94..f96297a 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -297,7 +297,9 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
   struct grub_net_network_level_interface *inf;
   char *name;
   char *ptr;
+  grub_err_t err;
   grub_net_network_level_address_t addr;
+  grub_net_network_level_netaddress_t netaddr;
 
   name = grub_malloc (grub_strlen (card->name)
 		      + GRUB_NET_MAX_STR_HWADDR_LEN
@@ -325,7 +327,17 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
       ptr += grub_strlen (ptr);
     }
   ptr = grub_stpcpy (ptr, ":link");
-  return grub_net_add_addr_real (name, card, &addr, hwaddr, 0);
+  inf = grub_net_add_addr_real (name, card, &addr, hwaddr, 0);
+  if (!inf)
+    return NULL;
+  netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+  netaddr.ipv6.base[0] = grub_cpu_to_be64_compile_time (0xfe80ULL << 48);
+  netaddr.ipv6.base[1] = 0;
+  netaddr.ipv6.masksize = 64;
+  err = grub_net_add_route (name, netaddr, inf);
+  if (err != GRUB_ERR_NONE)
+    return NULL;
+  return inf;
 }
 
 /* FIXME: allow to specify mac address.  */
-- 
2.1.0

[PATCH 3/3] net: fix ipv6 routing

[Josef Bacik]

Currently we cannot talk to ipv6 addresses outside of our local link area
because we don't setup our routes properly nor do we talk to the router
properly.  Currently net_ipv6_autoconf adds routes for the local link prefix and
ties it to the global link device.  This isn't helpful at all, we completely
drop the router part of the router advertisement.  So instead create a "default
route" flag in routes and create a new route with the local link prefix and the
router source address.

The second part of this problem is that the routing code thinks in ipv4 terms,
so it expects to find a route to the address we're looking for in our routing
table.  If we find a gateway then we just look for which interface matches the
network for the gateway.  This doesn't work for ipv6 since the router gives us
its local link address, so the gateway will find the local link interface,
instead of the global interface.  So to deal with this do a few things

1) Create a new "default route" flag for any router advertisements we get when
doing the ipv6 autoconf stuff.  Then we create a normal gateway route with the
default route flag set.

2) When looking for routes keep track of any default routes we find, and if we
don't find an exact route that matches we use the default route, which will set
the gateway to the router, and then we will find the local link interface that
matches this router.

3) Translate the local link interface to the global link interface.  We build
the ip packets based on the address on the interface, and this needs to have the
global address, not the local one.  So loop through our interfaces until we find
a global address with the same hwaddress as the local link interface and use
that instead.  This way we get the right addresses on our IP packets.

With this patch I can now do net_ipv6_autoconf and immediately talk to ipv6
addresses outside of my local network.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 grub-core/net/bootp.c                  |  2 +-
 grub-core/net/drivers/ieee1275/ofnet.c |  2 +-
 grub-core/net/icmp6.c                  | 50 ++++++++++------------
 grub-core/net/net.c                    | 76 +++++++++++++++++++++-------------
 include/grub/net.h                     | 28 ++++++++++++-
 5 files changed, 99 insertions(+), 59 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 37d1cfa..ed6cf44 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -83,7 +83,7 @@ parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
 	      grub_memcpy (&gw.ipv4, ptr, sizeof (gw.ipv4));
 	      rname = grub_xasprintf ("%s:default", name);
 	      if (rname)
-		grub_net_add_route_gw (rname, target, gw);
+		grub_net_add_route_gw (rname, target, gw, 0);
 	      grub_free (rname);
 	    }
 	  break;
diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index eea8e71..801ab1c 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -221,7 +221,7 @@ grub_ieee1275_parse_bootpath (const char *devpath, char *bootpath,
       target.ipv4.masksize = 0;
       rname = grub_xasprintf ("%s:default", ((*card)->name));
       if (rname)
-        grub_net_add_route_gw (rname, target, gateway_addr);
+        grub_net_add_route_gw (rname, target, gateway_addr, 0);
       else
         return grub_errno;
     }
diff --git a/grub-core/net/icmp6.c b/grub-core/net/icmp6.c
index 7953e68..fad04a9 100644
--- a/grub-core/net/icmp6.c
+++ b/grub-core/net/icmp6.c
@@ -373,7 +373,10 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
 	    if (ohdr->type == OPTION_PREFIX && ohdr->len == 4)
 	      {
 		struct prefix_option *opt = (struct prefix_option *) ptr;
+		struct grub_net_route *route;
 		struct grub_net_slaac_mac_list *slaac;
+
+		grub_net_network_level_netaddress_t netaddr;
 		if (!(opt->flags & FLAG_SLAAC)
 		    || (grub_be_to_cpu64 (opt->prefix[0]) >> 48) == 0xfe80
 		    || (grub_be_to_cpu32 (opt->preferred_lifetime)
@@ -391,18 +394,12 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
 		for (slaac = card->slaac_list; slaac; slaac = slaac->next)
 		  {
 		    grub_net_network_level_address_t addr;
-		    grub_net_network_level_netaddress_t netaddr;
-
 		    if (slaac->address.type
 			!= GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET)
 		      continue;
 		    addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
 		    addr.ipv6[0] = opt->prefix[0];
 		    addr.ipv6[1] = grub_net_ipv6_get_id (&slaac->address);
-		    netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
-		    netaddr.ipv6.base[0] = opt->prefix[0];
-		    netaddr.ipv6.base[1] = 0;
-		    netaddr.ipv6.masksize = 64;
 
 		    FOR_NET_NETWORK_LEVEL_INTERFACES (inf)
 		    {
@@ -410,29 +407,28 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
 			  && grub_net_addr_cmp (&inf->address, &addr) == 0)
 			break;
 		    }
-		    /* Update lease time if needed here once we have
-		       lease times.  */
-		    if (inf)
-		      continue;
+		    if (!inf)
+		      slaac->slaac_counter++;
+		  }
 
-		    grub_dprintf ("net", "creating slaac\n");
+		netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+		netaddr.ipv6.base[0] = opt->prefix[0];
+		netaddr.ipv6.base[1] = 0;
+		netaddr.ipv6.masksize = 64;
 
-		    {
-		      char *name;
-		      name = grub_xasprintf ("%s:%d",
-					     slaac->name, slaac->slaac_counter++);
-		      if (!name)
-			{
-			  grub_errno = GRUB_ERR_NONE;
-			  continue;
-			}
-		      inf = grub_net_add_addr (name, 
-					       card, &addr,
-					       &slaac->address, 0);
-		      grub_net_add_route (name, netaddr, inf);
-		      grub_free (name);
-		    }
-		  }
+		FOR_NET_ROUTES(route)
+		{
+		  if (!grub_memcmp(&route->gw, source, sizeof (route->gw)))
+		    break;
+		}
+		/* Update lease time if needed here once we have
+		   lease times.  */
+		if (route)
+		  continue;
+
+		grub_dprintf ("net", "creating default route\n");
+
+		grub_net_add_route_gw ("default6", netaddr, *source, 1);
 	      }
 	  }
 	if (ptr != nb->tail)
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index f96297a..185b635 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -37,21 +37,6 @@ GRUB_MOD_LICENSE ("GPLv3+");
 
 char *grub_net_default_server;
 
-struct grub_net_route
-{
-  struct grub_net_route *next;
-  struct grub_net_route **prev;
-  grub_net_network_level_netaddress_t target;
-  char *name;
-  struct grub_net_network_level_protocol *prot;
-  int is_gateway;
-  union
-  {
-    struct grub_net_network_level_interface *interface;
-    grub_net_network_level_address_t gw;
-  };
-};
-
 struct grub_net_route *grub_net_routes = NULL;
 struct grub_net_network_level_interface *grub_net_network_level_interfaces = NULL;
 struct grub_net_card *grub_net_cards = NULL;
@@ -422,14 +407,6 @@ grub_cmd_ipv6_autoconf (struct grub_command *cmd __attribute__ ((unused)),
   return err;
 }
 
-static inline void
-grub_net_route_register (struct grub_net_route *route)
-{
-  grub_list_push (GRUB_AS_LIST_P (&grub_net_routes),
-		  GRUB_AS_LIST (route));
-}
-
-#define FOR_NET_ROUTES(var) for (var = grub_net_routes; var; var = var->next)
 
 static int
 parse_ip (const char *val, grub_uint32_t *ip, const char **rest)
@@ -650,6 +627,11 @@ route_cmp (const struct grub_net_route *a, const struct grub_net_route *b)
 	return +1;
       if (a->target.ipv6.masksize < b->target.ipv6.masksize)
 	return -1;
+      /* We want to prefer non-default routes over default routes */
+      if (a->default_route > b->default_route)
+	return -1;
+      if (a->default_route < b->default_route)
+	return +1;
       break;
     case GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4:
       if (a->target.ipv4.masksize > b->target.ipv4.masksize)
@@ -661,6 +643,32 @@ route_cmp (const struct grub_net_route *a, const struct grub_net_route *b)
   return 0;
 }
 
+static struct grub_net_network_level_interface *
+grub_ipv6_global_interface (struct grub_net_network_level_interface *inf)
+{
+  grub_net_link_level_address_t hwaddr;
+  struct grub_net_network_level_interface *tmp;
+
+  /* We don't care about non ipv6 interfaces */
+  if (inf->address.type != GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6)
+    return inf;
+
+  /* If this isn't a link local interface then we're good to go */
+  if (inf->address.ipv6[0] != grub_cpu_to_be64_compile_time (0xfe80ULL << 48))
+    return inf;
+
+  grub_memcpy(&hwaddr, &inf->hwaddress, sizeof (inf->hwaddress));
+
+  FOR_NET_NETWORK_LEVEL_INTERFACES (tmp)
+  {
+    if (tmp->address.ipv6[0] == grub_cpu_to_be64_compile_time (0xfe80ULL << 48))
+      continue;
+    if (grub_net_hwaddr_cmp(&tmp->hwaddress, &hwaddr) == 0)
+      return tmp;
+  }
+  return inf;
+}
+
 grub_err_t
 grub_net_route_address (grub_net_network_level_address_t addr,
 			grub_net_network_level_address_t *gateway,
@@ -680,22 +688,29 @@ grub_net_route_address (grub_net_network_level_address_t addr,
   for (depth = 0; depth < routecnt + 2 && depth < GRUB_UINT_MAX; depth++)
     {
       struct grub_net_route *bestroute = NULL;
+      struct grub_net_route *defaultroute = NULL;
+
       FOR_NET_ROUTES(route)
       {
 	if (depth && prot != route->prot)
 	  continue;
+	if (!defaultroute && route->default_route)
+	  defaultroute = route;
 	if (!match_net (&route->target, &curtarget))
 	  continue;
 	if (route_cmp (route, bestroute) > 0)
 	  bestroute = route;
       }
-      if (bestroute == NULL)
-	return grub_error (GRUB_ERR_NET_NO_ROUTE,
-			   N_("destination unreachable"));
+      if (bestroute == NULL) {
+	if (defaultroute == NULL)
+	  return grub_error (GRUB_ERR_NET_NO_ROUTE,
+			     N_("destination unreachable"));
+	bestroute = defaultroute;
+      }
 
       if (!bestroute->is_gateway)
 	{
-	  *interf = bestroute->interface;
+	  *interf = grub_ipv6_global_interface(bestroute->interface);
 	  return GRUB_ERR_NONE;
 	}
       if (depth == 0)
@@ -1121,7 +1136,7 @@ grub_net_add_route (const char *name,
 grub_err_t
 grub_net_add_route_gw (const char *name,
 		       grub_net_network_level_netaddress_t target,
-		       grub_net_network_level_address_t gw)
+		       grub_net_network_level_address_t gw, int default_route)
 {
   struct grub_net_route *route;
 
@@ -1139,6 +1154,7 @@ grub_net_add_route_gw (const char *name,
   route->target = target;
   route->is_gateway = 1;
   route->gw = gw;
+  route->default_route = default_route;
 
   grub_net_route_register (route);
 
@@ -1164,7 +1180,7 @@ grub_cmd_addroute (struct grub_command *cmd __attribute__ ((unused)),
       err = grub_net_resolve_address (args[3], &gw);
       if (err)
 	return err;
-      return grub_net_add_route_gw (args[0], target, gw);
+      return grub_net_add_route_gw (args[0], target, gw, 0);
     }
   else
     {
@@ -1239,6 +1255,8 @@ grub_cmd_listroutes (struct grub_command *cmd __attribute__ ((unused)),
       }
     else
       grub_printf ("%s", route->interface->name);      
+    if (route->default_route)
+      grub_printf (" default route");
     grub_printf ("\n");
   }
   return GRUB_ERR_NONE;
diff --git a/include/grub/net.h b/include/grub/net.h
index 4571b72..4e98ddd 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -192,6 +192,22 @@ typedef struct grub_net_network_level_netaddress
   };
 } grub_net_network_level_netaddress_t;
 
+struct grub_net_route
+{
+  struct grub_net_route *next;
+  struct grub_net_route **prev;
+  grub_net_network_level_netaddress_t target;
+  char *name;
+  struct grub_net_network_level_protocol *prot;
+  int is_gateway;
+  int default_route;
+  union
+  {
+    struct grub_net_network_level_interface *interface;
+    grub_net_network_level_address_t gw;
+  };
+};
+
 #define FOR_PACKETS(cont,var) for (var = (cont).first; var; var = var->next)
 
 static inline grub_err_t
@@ -368,6 +384,16 @@ grub_net_card_unregister (struct grub_net_card *card);
 #define FOR_NET_CARDS_SAFE(var, next) for (var = grub_net_cards, next = (var ? var->next : 0); var; var = next, next = (var ? var->next : 0))
 
 
+extern struct grub_net_route *grub_net_routes;
+
+static inline void
+grub_net_route_register (struct grub_net_route *route)
+{
+  grub_list_push (GRUB_AS_LIST_P (&grub_net_routes),
+		  GRUB_AS_LIST (route));
+}
+
+#define FOR_NET_ROUTES(var) for (var = grub_net_routes; var; var = var->next)
 struct grub_net_session *
 grub_net_open_tcp (char *address, grub_uint16_t port);
 
@@ -393,7 +419,7 @@ grub_net_add_route (const char *name,
 grub_err_t
 grub_net_add_route_gw (const char *name,
 		       grub_net_network_level_netaddress_t target,
-		       grub_net_network_level_address_t gw);
+		       grub_net_network_level_address_t gw, int default_route);
 
 
 #define GRUB_NET_BOOTP_MAC_ADDR_LEN	16
-- 
2.1.0

Re: [PATCH 1/3] efinet: handle get_status() properly

[Andrei Borzenkov]

В Wed, 5 Aug 2015 14:36:37 -0400
Josef Bacik <jbacik@fb.com> пишет:

> The EFI SNP documentation isn't super clear on the value that is returned in
> txbuf when calling into GetStatus.  The documentation says its the pointer to
> the recycle buffer, but the documentation for Transmit() says that it should be
> the pointer to the buffer that we transmitted.

Actually it says "Recycled transmit buffer address" and further
"GetStatus() until the transmitted buffer shows up in the recycled
transmit buffer" so it looks reasonably clear to me.

>                                               On the boxes I'm using it's just
> a random pointer (usually 0x1).  It is definitely transmitting stuff, but the
> get_status call is not returning the pointer to the txbuf we passed in.

Which sounds like firmware bug. To be sure - you observe it also using
current GIT master?

>                                                                        Looking
> at a few EFI implementations and other SNP drivers it seems like there is
> confusion everywhere on this.  So since we only transmit one buffer at a time,
> just assume that a non-NULL txbuf means that our transmit happened properly.
> With this patch I can now do networking on our EFI enabled boxes.  Thanks,
> 
> cc: Peter Jones <pjones@redhat.com>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  grub-core/net/drivers/efi/efinet.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
> index f27a117..4d3f8aa 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -47,19 +47,11 @@ send_card_buffer (struct grub_net_card *dev,
>  	if (st != GRUB_EFI_SUCCESS)
>  	  return grub_error (GRUB_ERR_IO,
>  			     N_("couldn't send network packet"));
> -	if (txbuf == dev->txbuf)
> +	if (txbuf)
>  	  {
>  	    dev->txbusy = 0;
>  	    break;
>  	  }
> -	if (txbuf)
> -	  {
> -	    st = efi_call_7 (net->transmit, net, 0, dev->last_pkt_size,
> -			     dev->txbuf, NULL, NULL, NULL);
> -	    if (st != GRUB_EFI_SUCCESS)
> -	      return grub_error (GRUB_ERR_IO,
> -				 N_("couldn't send network packet"));
> -	  }
>  	if (limit_time < grub_get_time_ms ())
>  	  return grub_error (GRUB_ERR_TIMEOUT,
>  			     N_("couldn't send network packet"));
> @@ -84,8 +76,9 @@ send_card_buffer (struct grub_net_card *dev,
>       we run in the GRUB_ERR_TIMEOUT case above.
>       Perhaps a timeout in the FW has discarded the recycle buffer.
>     */
> +  txbuf = NULL;
>    st = efi_call_3 (net->get_status, net, 0, &txbuf);
> -  dev->txbusy = !(st == GRUB_EFI_SUCCESS && txbuf == dev->txbuf);
> +  dev->txbusy = !(st == GRUB_EFI_SUCCESS && txbuf);
>  
>    return GRUB_ERR_NONE;
>  }

Re: [PATCH 1/3] efinet: handle get_status() properly

[Josef Bacik]

On 08/05/2015 04:04 PM, Andrei Borzenkov wrote:
> В Wed, 5 Aug 2015 14:36:37 -0400
> Josef Bacik <jbacik@fb.com> пишет:
>
>> The EFI SNP documentation isn't super clear on the value that is returned in
>> txbuf when calling into GetStatus.  The documentation says its the pointer to
>> the recycle buffer, but the documentation for Transmit() says that it should be
>> the pointer to the buffer that we transmitted.
>
> Actually it says "Recycled transmit buffer address" and further
> "GetStatus() until the transmitted buffer shows up in the recycled
> transmit buffer" so it looks reasonably clear to me.
>
>>                                                On the boxes I'm using it's just
>> a random pointer (usually 0x1).  It is definitely transmitting stuff, but the
>> get_status call is not returning the pointer to the txbuf we passed in.
>
> Which sounds like firmware bug. To be sure - you observe it also using
> current GIT master?
>

This is on git master as of last week, so I have your latest patch

efinet: enable hardware filters when opening interface

and it was still happening.  I know what Transmit() says, but 
GetStatus() says it'll just be a pointer to the recycled transmit buffer 
address, which to me could mean the pointer to whatever the internal 
queue was being used by UEFI.  Anyway that's not important, what is 
important is that the current code doesn't work with hardware that 
exists in the wild.  If it's a firmware bug then fine, what do users do 
if they have buggy firmware that isn't being updated anymore?  I think 
making grub more tolerant to crappy firmware is a good thing.  Thanks,

Josef

Re: [PATCH 1/3] efinet: handle get_status() properly

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

This patch improperly assumes that GRUB is the only thing in EFI that
transmits. Your patch surely fixed your machine but likely breaks some
other machines. Could you instead make an explicit check for (void *)1 and
add a comment on which machine it's necessary?
Le 5 août 2015 10:28 PM, "Josef Bacik" <jbacik@fb.com> a écrit :

> On 08/05/2015 04:04 PM, Andrei Borzenkov wrote:
>
>> В Wed, 5 Aug 2015 14:36:37 -0400
>> Josef Bacik <jbacik@fb.com> пишет:
>>
>> The EFI SNP documentation isn't super clear on the value that is returned
>>> in
>>> txbuf when calling into GetStatus.  The documentation says its the
>>> pointer to
>>> the recycle buffer, but the documentation for Transmit() says that it
>>> should be
>>> the pointer to the buffer that we transmitted.
>>>
>>
>> Actually it says "Recycled transmit buffer address" and further
>> "GetStatus() until the transmitted buffer shows up in the recycled
>> transmit buffer" so it looks reasonably clear to me.
>>
>>                                                On the boxes I'm using
>>> it's just
>>> a random pointer (usually 0x1).  It is definitely transmitting stuff,
>>> but the
>>> get_status call is not returning the pointer to the txbuf we passed in.
>>>
>>
>> Which sounds like firmware bug. To be sure - you observe it also using
>> current GIT master?
>>
>>
> This is on git master as of last week, so I have your latest patch
>
> efinet: enable hardware filters when opening interface
>
> and it was still happening.  I know what Transmit() says, but GetStatus()
> says it'll just be a pointer to the recycled transmit buffer address, which
> to me could mean the pointer to whatever the internal queue was being used
> by UEFI.  Anyway that's not important, what is important is that the
> current code doesn't work with hardware that exists in the wild.  If it's a
> firmware bug then fine, what do users do if they have buggy firmware that
> isn't being updated anymore?  I think making grub more tolerant to crappy
> firmware is a good thing.  Thanks,
>
> Josef
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 3131 bytes --]

Re: [PATCH 1/3] efinet: handle get_status() properly

[Josef Bacik]

On 08/05/2015 04:32 PM, Vladimir 'phcoder' Serbinenko wrote:
> This patch improperly assumes that GRUB is the only thing in EFI that
> transmits. Your patch surely fixed your machine but likely breaks some
> other machines. Could you instead make an explicit check for (void *)1
> and add a comment on which machine it's necessary?
>

Yeah this is kind of a crap trade-off I know.  The problem is this is 
just on one box I'm testing with, we've got _a metric shit ton_ of 
boxes, if one of them returns 0x2 suddenly it can't be provisioned.  I 
realize this is racey with other things on UEFI doing stuff, but I don't 
have a better answer.  Maybe a range check for obviously bogus 
addresses?  Or maybe once we get a non-NULL from GetStatus() we call it 
again until we get a NULL from GetStatus()?  Thanks,

Josef

Re: [PATCH 1/3] efinet: handle get_status() properly

[Josef Bacik]

On 08/05/2015 04:39 PM, Josef Bacik wrote:
> On 08/05/2015 04:32 PM, Vladimir 'phcoder' Serbinenko wrote:
>> This patch improperly assumes that GRUB is the only thing in EFI that
>> transmits. Your patch surely fixed your machine but likely breaks some
>> other machines. Could you instead make an explicit check for (void *)1
>> and add a comment on which machine it's necessary?
>>
>
> Yeah this is kind of a crap trade-off I know.  The problem is this is
> just on one box I'm testing with, we've got _a metric shit ton_ of
> boxes, if one of them returns 0x2 suddenly it can't be provisioned.  I
> realize this is racey with other things on UEFI doing stuff, but I don't
> have a better answer.  Maybe a range check for obviously bogus
> addresses?  Or maybe once we get a non-NULL from GetStatus() we call it
> again until we get a NULL from GetStatus()?  Thanks,
>

Could also just add a variable along the lines of 
"net_known_shitty_efi_firmware" and only do this if that variable is 
set, that way it's the users choice to work around this or not.  Thanks,

Josef

Re: [PATCH 1/3] efinet: handle get_status() properly

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

Making user aware of such deeply technical stuff is almost always bad
thing. Moreover it's not always easy to set this variable early enough
Le 5 août 2015 10:50 PM, "Josef Bacik" <jbacik@fb.com> a écrit :

> On 08/05/2015 04:39 PM, Josef Bacik wrote:
>
>> On 08/05/2015 04:32 PM, Vladimir 'phcoder' Serbinenko wrote:
>>
>>> This patch improperly assumes that GRUB is the only thing in EFI that
>>> transmits. Your patch surely fixed your machine but likely breaks some
>>> other machines. Could you instead make an explicit check for (void *)1
>>> and add a comment on which machine it's necessary?
>>>
>>>
>> Yeah this is kind of a crap trade-off I know.  The problem is this is
>> just on one box I'm testing with, we've got _a metric shit ton_ of
>> boxes, if one of them returns 0x2 suddenly it can't be provisioned.  I
>> realize this is racey with other things on UEFI doing stuff, but I don't
>> have a better answer.  Maybe a range check for obviously bogus
>> addresses?  Or maybe once we get a non-NULL from GetStatus() we call it
>> again until we get a NULL from GetStatus()?  Thanks,
>>
>>
> Could also just add a variable along the lines of
> "net_known_shitty_efi_firmware" and only do this if that variable is set,
> that way it's the users choice to work around this or not.  Thanks,
>
> Josef
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2204 bytes --]

Re: [PATCH 1/3] efinet: handle get_status() properly

[Seth Goldberg]

> Anyway that's not important, what is
> important is that the current code doesn't work with hardware that
> exists in the wild.  If it's a firmware bug then fine, what do users do
> if they have buggy firmware that isn't being updated anymore?  I think
> making grub more tolerant to crappy firmware is a good thing.  Thanks,

   Absolutely agree.

  --S

Re: [PATCH 1/3] efinet: handle get_status() properly

[Andrei Borzenkov]

В Wed, 5 Aug 2015 22:32:13 +0200
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:

> This patch improperly assumes that GRUB is the only thing in EFI that
> transmits.

Actually since recently we try to ensure that grub *is* the only user
of network interface. 

>           Your patch surely fixed your machine but likely breaks some
> other machines. Could you instead make an explicit check for (void *)1 and
> add a comment on which machine it's necessary?

Yes, this patch should add verbose comment to code explaining a) what
problem it tries to fix and b) why ignoring EFI specification is
justified here. Also mention actual hardware/firmware implementation
where this bug happens for future reference.

Looking at other implementations gPXE opens SNP non-exclusively and
explicitly checks returned address. That corresponds to what grub
did in the past. iPXE opens SNP exclusively and assumes anything !=
NULL means transmit completed (it seems to start off with gPXE model).

> Le 5 août 2015 10:28 PM, "Josef Bacik" <jbacik@fb.com> a écrit :
> 
> > On 08/05/2015 04:04 PM, Andrei Borzenkov wrote:
> >
> >> В Wed, 5 Aug 2015 14:36:37 -0400
> >> Josef Bacik <jbacik@fb.com> пишет:
> >>
> >> The EFI SNP documentation isn't super clear on the value that is returned
> >>> in
> >>> txbuf when calling into GetStatus.  The documentation says its the
> >>> pointer to
> >>> the recycle buffer, but the documentation for Transmit() says that it
> >>> should be
> >>> the pointer to the buffer that we transmitted.
> >>>
> >>
> >> Actually it says "Recycled transmit buffer address" and further
> >> "GetStatus() until the transmitted buffer shows up in the recycled
> >> transmit buffer" so it looks reasonably clear to me.
> >>
> >>                                                On the boxes I'm using
> >>> it's just
> >>> a random pointer (usually 0x1).  It is definitely transmitting stuff,
> >>> but the
> >>> get_status call is not returning the pointer to the txbuf we passed in.
> >>>
> >>
> >> Which sounds like firmware bug. To be sure - you observe it also using
> >> current GIT master?
> >>
> >>
> > This is on git master as of last week, so I have your latest patch
> >
> > efinet: enable hardware filters when opening interface
> >
> > and it was still happening.  I know what Transmit() says, but GetStatus()
> > says it'll just be a pointer to the recycled transmit buffer address, which
> > to me could mean the pointer to whatever the internal queue was being used
> > by UEFI.  Anyway that's not important, what is important is that the
> > current code doesn't work with hardware that exists in the wild.  If it's a
> > firmware bug then fine, what do users do if they have buggy firmware that
> > isn't being updated anymore?  I think making grub more tolerant to crappy
> > firmware is a good thing.  Thanks,
> >
> > Josef
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >

Re: [PATCH 1/3] efinet: handle get_status() properly

[Josef Bacik]

On 08/05/2015 11:42 PM, Andrei Borzenkov wrote:
> В Wed, 5 Aug 2015 22:32:13 +0200
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
>
>> This patch improperly assumes that GRUB is the only thing in EFI that
>> transmits.
>
> Actually since recently we try to ensure that grub *is* the only user
> of network interface.
>
>>            Your patch surely fixed your machine but likely breaks some
>> other machines. Could you instead make an explicit check for (void *)1 and
>> add a comment on which machine it's necessary?
>
> Yes, this patch should add verbose comment to code explaining a) what
> problem it tries to fix and b) why ignoring EFI specification is
> justified here. Also mention actual hardware/firmware implementation
> where this bug happens for future reference.
>
> Looking at other implementations gPXE opens SNP non-exclusively and
> explicitly checks returned address. That corresponds to what grub
> did in the past. iPXE opens SNP exclusively and assumes anything !=
> NULL means transmit completed (it seems to start off with gPXE model).

Ok I'll add a comment and resend the patch as it is.  Thanks,

Josef

[PATCH V2] efinet: handle get_status() on buggy firmware properly

[Josef Bacik]

The EFI spec indicates that get_status() should return the address of the buffer
we passed into transmit to indicate the the buffer was transmitted.  However we
have boxes where the firmware returns some arbitrary address instead, which
makes grub think that we've not sent anything.  So since we have the SNP stuff
opened in exclusive mode just assume any non-NULL txbuf means that our transmit
occurred properly.  This makes grub able to do its networking stuff properly on
our broken firmware.  Thanks,

cc: Peter Jones <pjones@redhat.com>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
V1->V2:
-updated the changelog to be more clear about what we are fixing.
-added a comment to the code to indicate why it is safe to only check for
 non-NULL txbufs and why we need to do it.

 grub-core/net/drivers/efi/efinet.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index f27a117..692d5ad 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -47,19 +47,19 @@ send_card_buffer (struct grub_net_card *dev,
 	if (st != GRUB_EFI_SUCCESS)
 	  return grub_error (GRUB_ERR_IO,
 			     N_("couldn't send network packet"));
-	if (txbuf == dev->txbuf)
+	/*
+	   Some buggy firmware could return an arbitrary address instead of the
+	   txbuf address we trasmitted, so just check that txbuf is non NULL
+	   for success.  This is ok because we open the SNP protocol in
+	   exclusive mode so we know we're the only ones transmitting on this
+	   box and since we only transmit one packet at a time we know our
+	   transmit was successfull.
+	 */
+	if (txbuf)
 	  {
 	    dev->txbusy = 0;
 	    break;
 	  }
-	if (txbuf)
-	  {
-	    st = efi_call_7 (net->transmit, net, 0, dev->last_pkt_size,
-			     dev->txbuf, NULL, NULL, NULL);
-	    if (st != GRUB_EFI_SUCCESS)
-	      return grub_error (GRUB_ERR_IO,
-				 N_("couldn't send network packet"));
-	  }
 	if (limit_time < grub_get_time_ms ())
 	  return grub_error (GRUB_ERR_TIMEOUT,
 			     N_("couldn't send network packet"));
@@ -84,8 +84,9 @@ send_card_buffer (struct grub_net_card *dev,
      we run in the GRUB_ERR_TIMEOUT case above.
      Perhaps a timeout in the FW has discarded the recycle buffer.
    */
+  txbuf = NULL;
   st = efi_call_3 (net->get_status, net, 0, &txbuf);
-  dev->txbusy = !(st == GRUB_EFI_SUCCESS && txbuf == dev->txbuf);
+  dev->txbusy = !(st == GRUB_EFI_SUCCESS && txbuf);
 
   return GRUB_ERR_NONE;
 }
-- 
1.8.1

Re: [PATCH 2/3] net: add local route when creating link local ipv6 interface

[Andrei Borzenkov]

On Wed, Aug 5, 2015 at 9:36 PM, Josef Bacik <jbacik@fb.com> wrote:
> In order to talk to link local ipv6 addresses we need to have a route out of the
> link local interface for this to work.  Thanks,
>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  grub-core/net/net.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 21a4e94..f96297a 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -297,7 +297,9 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
>    struct grub_net_network_level_interface *inf;
>    char *name;
>    char *ptr;
> +  grub_err_t err;
>    grub_net_network_level_address_t addr;
> +  grub_net_network_level_netaddress_t netaddr;
>
>    name = grub_malloc (grub_strlen (card->name)
>                       + GRUB_NET_MAX_STR_HWADDR_LEN
> @@ -325,7 +327,17 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
>        ptr += grub_strlen (ptr);
>      }
>    ptr = grub_stpcpy (ptr, ":link");
> -  return grub_net_add_addr_real (name, card, &addr, hwaddr, 0);
> +  inf = grub_net_add_addr_real (name, card, &addr, hwaddr, 0);
> +  if (!inf)
> +    return NULL;
> +  netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> +  netaddr.ipv6.base[0] = grub_cpu_to_be64_compile_time (0xfe80ULL << 48);
> +  netaddr.ipv6.base[1] = 0;
> +  netaddr.ipv6.masksize = 64;
> +  err = grub_net_add_route (name, netaddr, inf);

In case of multiple active interfaces it effectively directs all
link-local traffic to one interface only. It does not sound right.

Link-local is link local by definition so we do not really need
routing table at all; but we do need to know via which card to send
it. For the case of single active card we do not have choice, so can
proceed directly to sending.

For multiple active cards one possibility is to start neighbor
detection for link-local on all configured cards. Another to add
explicit host route to each received address pointing to interface via
which it was receieved.


> +  if (err != GRUB_ERR_NONE)
> +    return NULL;
> +  return inf;
>  }
>
>  /* FIXME: allow to specify mac address.  */
> --
> 2.1.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH V2] efinet: handle get_status() on buggy firmware properly

[Andrei Borzenkov]

В Thu,  6 Aug 2015 10:49:46 -0700
Josef Bacik <jbacik@fb.com> пишет:

> The EFI spec indicates that get_status() should return the address of the buffer
> we passed into transmit to indicate the the buffer was transmitted.  However we
> have boxes where the firmware returns some arbitrary address instead, which
> makes grub think that we've not sent anything.  So since we have the SNP stuff
> opened in exclusive mode just assume any non-NULL txbuf means that our transmit
> occurred properly.  This makes grub able to do its networking stuff properly on
> our broken firmware.  Thanks,
> 

Committed. Let's see if someone screams.

> cc: Peter Jones <pjones@redhat.com>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
> V1->V2:
> -updated the changelog to be more clear about what we are fixing.
> -added a comment to the code to indicate why it is safe to only check for
>  non-NULL txbufs and why we need to do it.
> 
>  grub-core/net/drivers/efi/efinet.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
> index f27a117..692d5ad 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -47,19 +47,19 @@ send_card_buffer (struct grub_net_card *dev,
>  	if (st != GRUB_EFI_SUCCESS)
>  	  return grub_error (GRUB_ERR_IO,
>  			     N_("couldn't send network packet"));
> -	if (txbuf == dev->txbuf)
> +	/*
> +	   Some buggy firmware could return an arbitrary address instead of the
> +	   txbuf address we trasmitted, so just check that txbuf is non NULL
> +	   for success.  This is ok because we open the SNP protocol in
> +	   exclusive mode so we know we're the only ones transmitting on this
> +	   box and since we only transmit one packet at a time we know our
> +	   transmit was successfull.
> +	 */
> +	if (txbuf)
>  	  {
>  	    dev->txbusy = 0;
>  	    break;
>  	  }
> -	if (txbuf)
> -	  {
> -	    st = efi_call_7 (net->transmit, net, 0, dev->last_pkt_size,
> -			     dev->txbuf, NULL, NULL, NULL);
> -	    if (st != GRUB_EFI_SUCCESS)
> -	      return grub_error (GRUB_ERR_IO,
> -				 N_("couldn't send network packet"));
> -	  }
>  	if (limit_time < grub_get_time_ms ())
>  	  return grub_error (GRUB_ERR_TIMEOUT,
>  			     N_("couldn't send network packet"));
> @@ -84,8 +84,9 @@ send_card_buffer (struct grub_net_card *dev,
>       we run in the GRUB_ERR_TIMEOUT case above.
>       Perhaps a timeout in the FW has discarded the recycle buffer.
>     */
> +  txbuf = NULL;
>    st = efi_call_3 (net->get_status, net, 0, &txbuf);
> -  dev->txbusy = !(st == GRUB_EFI_SUCCESS && txbuf == dev->txbuf);
> +  dev->txbusy = !(st == GRUB_EFI_SUCCESS && txbuf);
>  
>    return GRUB_ERR_NONE;
>  }

Re: [PATCH 3/3] net: fix ipv6 routing

[Andrei Borzenkov]

В Wed, 5 Aug 2015 14:36:39 -0400
Josef Bacik <jbacik@fb.com> пишет:

> Currently we cannot talk to ipv6 addresses outside of our local link area
> because we don't setup our routes properly nor do we talk to the router
> properly.  Currently net_ipv6_autoconf adds routes for the local link prefix and
> ties it to the global link device.  This isn't helpful at all, we completely
> drop the router part of the router advertisement.  So instead create a "default
> route" flag in routes and create a new route with the local link prefix and the
> router source address.
> 

Note that RA does not always mean default route. You need to check
router lifetime to decide whether to use it as default route.

> The second part of this problem is that the routing code thinks in ipv4 terms,
> so it expects to find a route to the address we're looking for in our routing
> table.  If we find a gateway then we just look for which interface matches the
> network for the gateway.  This doesn't work for ipv6 since the router gives us
> its local link address, so the gateway will find the local link interface,
> instead of the global interface.  So to deal with this do a few things
> 

I am afraid it cannot be solved on routing table level. We need to bite
the bullet and add notion of interface zone and associate each link
local IPv6 address with interface it comes from (or sent to). This
automatically solves also the problem in your other patch as well as
this one by eliminating need to (attempt to) route link local in the
first place - it is simply sent to interface it is associated with.

It also means we probably need to support IPv6%scope notation for
addresses.

> 1) Create a new "default route" flag for any router advertisements we get when
> doing the ipv6 autoconf stuff.  Then we create a normal gateway route with the
> default route flag set.
> 
> 2) When looking for routes keep track of any default routes we find, and if we
> don't find an exact route that matches we use the default route, which will set
> the gateway to the router, and then we will find the local link interface that
> matches this router.
> 
> 3) Translate the local link interface to the global link interface.  We build
> the ip packets based on the address on the interface, and this needs to have the
> global address, not the local one.  So loop through our interfaces until we find
> a global address with the same hwaddress as the local link interface and use
> that instead.  This way we get the right addresses on our IP packets.
> 

If I read RFC6724 correctly, we actually must prefer link-local
source address when speaking with link-local destination.

> With this patch I can now do net_ipv6_autoconf and immediately talk to ipv6
> addresses outside of my local network.  Thanks,
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  grub-core/net/bootp.c                  |  2 +-
>  grub-core/net/drivers/ieee1275/ofnet.c |  2 +-
>  grub-core/net/icmp6.c                  | 50 ++++++++++------------
>  grub-core/net/net.c                    | 76 +++++++++++++++++++++-------------
>  include/grub/net.h                     | 28 ++++++++++++-
>  5 files changed, 99 insertions(+), 59 deletions(-)
> 
...
> diff --git a/grub-core/net/icmp6.c b/grub-core/net/icmp6.c
> index 7953e68..fad04a9 100644
> --- a/grub-core/net/icmp6.c
> +++ b/grub-core/net/icmp6.c
> @@ -373,7 +373,10 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
>  	    if (ohdr->type == OPTION_PREFIX && ohdr->len == 4)
>  	      {
>  		struct prefix_option *opt = (struct prefix_option *) ptr;
> +		struct grub_net_route *route;
>  		struct grub_net_slaac_mac_list *slaac;
> +
> +		grub_net_network_level_netaddress_t netaddr;
>  		if (!(opt->flags & FLAG_SLAAC)
>  		    || (grub_be_to_cpu64 (opt->prefix[0]) >> 48) == 0xfe80
>  		    || (grub_be_to_cpu32 (opt->preferred_lifetime)
> @@ -391,18 +394,12 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
>  		for (slaac = card->slaac_list; slaac; slaac = slaac->next)
>  		  {
>  		    grub_net_network_level_address_t addr;
> -		    grub_net_network_level_netaddress_t netaddr;
> -
>  		    if (slaac->address.type
>  			!= GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET)
>  		      continue;
>  		    addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
>  		    addr.ipv6[0] = opt->prefix[0];
>  		    addr.ipv6[1] = grub_net_ipv6_get_id (&slaac->address);
> -		    netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> -		    netaddr.ipv6.base[0] = opt->prefix[0];
> -		    netaddr.ipv6.base[1] = 0;
> -		    netaddr.ipv6.masksize = 64;
>  
>  		    FOR_NET_NETWORK_LEVEL_INTERFACES (inf)
>  		    {
> @@ -410,29 +407,28 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
>  			  && grub_net_addr_cmp (&inf->address, &addr) == 0)
>  			break;
>  		    }
> -		    /* Update lease time if needed here once we have
> -		       lease times.  */
> -		    if (inf)
> -		      continue;
> +		    if (!inf)
> +		      slaac->slaac_counter++;
> +		  }
>  
> -		    grub_dprintf ("net", "creating slaac\n");
> +		netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> +		netaddr.ipv6.base[0] = opt->prefix[0];
> +		netaddr.ipv6.base[1] = 0;
> +		netaddr.ipv6.masksize = 64;
>  
> -		    {
> -		      char *name;
> -		      name = grub_xasprintf ("%s:%d",
> -					     slaac->name, slaac->slaac_counter++);
> -		      if (!name)
> -			{
> -			  grub_errno = GRUB_ERR_NONE;
> -			  continue;
> -			}
> -		      inf = grub_net_add_addr (name, 
> -					       card, &addr,
> -					       &slaac->address, 0);
> -		      grub_net_add_route (name, netaddr, inf);
> -		      grub_free (name);
> -		    }
> -		  }

This kills SLAAC. Where global addresses now come from? You probably
get them from DHCPv6 but it does not mean we should drop SLAAC.

> +		FOR_NET_ROUTES(route)
> +		{
> +		  if (!grub_memcmp(&route->gw, source, sizeof (route->gw)))
> +		    break;
> +		}
> +		/* Update lease time if needed here once we have
> +		   lease times.  */
> +		if (route)
> +		  continue;
> +
> +		grub_dprintf ("net", "creating default route\n");
> +
> +		grub_net_add_route_gw ("default6", netaddr, *source, 1);
>  	      }
>  	  }
>  	if (ptr != nb->tail)

Re: [PATCH 3/3] net: fix ipv6 routing

[Josef Bacik]

On 08/09/2015 10:58 AM, Andrei Borzenkov wrote:
> В Wed, 5 Aug 2015 14:36:39 -0400
> Josef Bacik <jbacik@fb.com> пишет:
>
>> Currently we cannot talk to ipv6 addresses outside of our local link area
>> because we don't setup our routes properly nor do we talk to the router
>> properly.  Currently net_ipv6_autoconf adds routes for the local link prefix and
>> ties it to the global link device.  This isn't helpful at all, we completely
>> drop the router part of the router advertisement.  So instead create a "default
>> route" flag in routes and create a new route with the local link prefix and the
>> router source address.
>>
>
> Note that RA does not always mean default route. You need to check
> router lifetime to decide whether to use it as default route.

I've been reading the spec a lot recently and I couldn't figure out a 
way to differentiate between a default route and just another route. 
 From my reading if we get multipe RA's we're supposed to just round 
robin through them until we get the one that works, obviously taking 
into account when the router lifetime expires and such.  We don't take 
into account the lifetime at all except to see if it is set to 0, 
otherwise we never expire anything.  We should probably build this out 
in the future, but for now just picking whichever router comes first as 
the default route will work most of the time.

>
>> The second part of this problem is that the routing code thinks in ipv4 terms,
>> so it expects to find a route to the address we're looking for in our routing
>> table.  If we find a gateway then we just look for which interface matches the
>> network for the gateway.  This doesn't work for ipv6 since the router gives us
>> its local link address, so the gateway will find the local link interface,
>> instead of the global interface.  So to deal with this do a few things
>>
>
> I am afraid it cannot be solved on routing table level. We need to bite
> the bullet and add notion of interface zone and associate each link
> local IPv6 address with interface it comes from (or sent to). This
> automatically solves also the problem in your other patch as well as
> this one by eliminating need to (attempt to) route link local in the
> first place - it is simply sent to interface it is associated with.
>
> It also means we probably need to support IPv6%scope notation for
> addresses.

I'll try and work something out that is better than this patch.

>
>> 1) Create a new "default route" flag for any router advertisements we get when
>> doing the ipv6 autoconf stuff.  Then we create a normal gateway route with the
>> default route flag set.
>>
>> 2) When looking for routes keep track of any default routes we find, and if we
>> don't find an exact route that matches we use the default route, which will set
>> the gateway to the router, and then we will find the local link interface that
>> matches this router.
>>
>> 3) Translate the local link interface to the global link interface.  We build
>> the ip packets based on the address on the interface, and this needs to have the
>> global address, not the local one.  So loop through our interfaces until we find
>> a global address with the same hwaddress as the local link interface and use
>> that instead.  This way we get the right addresses on our IP packets.
>>
>
> If I read RFC6724 correctly, we actually must prefer link-local
> source address when speaking with link-local destination.

Right if we're talking to a link-local destination, but when talking to 
a global link destination the format is

src-ip: global address for the interface
dst-ip: global address for destination
src-mac: our mac
dst-mac: the mac of the default route

Which is why I did that weird translation.

>
>> With this patch I can now do net_ipv6_autoconf and immediately talk to ipv6
>> addresses outside of my local network.  Thanks,
>>
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>> ---
>>   grub-core/net/bootp.c                  |  2 +-
>>   grub-core/net/drivers/ieee1275/ofnet.c |  2 +-
>>   grub-core/net/icmp6.c                  | 50 ++++++++++------------
>>   grub-core/net/net.c                    | 76 +++++++++++++++++++++-------------
>>   include/grub/net.h                     | 28 ++++++++++++-
>>   5 files changed, 99 insertions(+), 59 deletions(-)
>>
> ...
>> diff --git a/grub-core/net/icmp6.c b/grub-core/net/icmp6.c
>> index 7953e68..fad04a9 100644
>> --- a/grub-core/net/icmp6.c
>> +++ b/grub-core/net/icmp6.c
>> @@ -373,7 +373,10 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
>>   	    if (ohdr->type == OPTION_PREFIX && ohdr->len == 4)
>>   	      {
>>   		struct prefix_option *opt = (struct prefix_option *) ptr;
>> +		struct grub_net_route *route;
>>   		struct grub_net_slaac_mac_list *slaac;
>> +
>> +		grub_net_network_level_netaddress_t netaddr;
>>   		if (!(opt->flags & FLAG_SLAAC)
>>   		    || (grub_be_to_cpu64 (opt->prefix[0]) >> 48) == 0xfe80
>>   		    || (grub_be_to_cpu32 (opt->preferred_lifetime)
>> @@ -391,18 +394,12 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
>>   		for (slaac = card->slaac_list; slaac; slaac = slaac->next)
>>   		  {
>>   		    grub_net_network_level_address_t addr;
>> -		    grub_net_network_level_netaddress_t netaddr;
>> -
>>   		    if (slaac->address.type
>>   			!= GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET)
>>   		      continue;
>>   		    addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
>>   		    addr.ipv6[0] = opt->prefix[0];
>>   		    addr.ipv6[1] = grub_net_ipv6_get_id (&slaac->address);
>> -		    netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
>> -		    netaddr.ipv6.base[0] = opt->prefix[0];
>> -		    netaddr.ipv6.base[1] = 0;
>> -		    netaddr.ipv6.masksize = 64;
>>
>>   		    FOR_NET_NETWORK_LEVEL_INTERFACES (inf)
>>   		    {
>> @@ -410,29 +407,28 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
>>   			  && grub_net_addr_cmp (&inf->address, &addr) == 0)
>>   			break;
>>   		    }
>> -		    /* Update lease time if needed here once we have
>> -		       lease times.  */
>> -		    if (inf)
>> -		      continue;
>> +		    if (!inf)
>> +		      slaac->slaac_counter++;
>> +		  }
>>
>> -		    grub_dprintf ("net", "creating slaac\n");
>> +		netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
>> +		netaddr.ipv6.base[0] = opt->prefix[0];
>> +		netaddr.ipv6.base[1] = 0;
>> +		netaddr.ipv6.masksize = 64;
>>
>> -		    {
>> -		      char *name;
>> -		      name = grub_xasprintf ("%s:%d",
>> -					     slaac->name, slaac->slaac_counter++);
>> -		      if (!name)
>> -			{
>> -			  grub_errno = GRUB_ERR_NONE;
>> -			  continue;
>> -			}
>> -		      inf = grub_net_add_addr (name,
>> -					       card, &addr,
>> -					       &slaac->address, 0);
>> -		      grub_net_add_route (name, netaddr, inf);
>> -		      grub_free (name);
>> -		    }
>> -		  }
>
> This kills SLAAC. Where global addresses now come from? You probably
> get them from DHCPv6 but it does not mean we should drop SLAAC.

Eesh I just ripped that out didn't I?  Sorry about that, I'll rework 
this again.  I think I'll do like you said above, just kind of create an 
interface with different scopes and that way if you use both slaac and 
dhcp and get the same address we don't end up with two different 
interfaces.  And then I'll come up with something to tie the routers to 
the interfaces and rework the route stuff so it is a bit cleaner.  This 
will probably be towards the end of the week, I've found some weird 
timeout problem in the TCP stack I'm trying to track down, once I've 
figured that out I'll rework this patch.  Thanks,

Josef

Re: [PATCH 3/3] net: fix ipv6 routing

[Andrei Borzenkov]

On Mon, Aug 10, 2015 at 5:00 PM, Josef Bacik <jbacik@fb.com> wrote:
> On 08/09/2015 10:58 AM, Andrei Borzenkov wrote:
>>
>> В Wed, 5 Aug 2015 14:36:39 -0400
>> Josef Bacik <jbacik@fb.com> пишет:
>>
>>> Currently we cannot talk to ipv6 addresses outside of our local link area
>>> because we don't setup our routes properly nor do we talk to the router
>>> properly.  Currently net_ipv6_autoconf adds routes for the local link
>>> prefix and
>>> ties it to the global link device.  This isn't helpful at all, we
>>> completely
>>> drop the router part of the router advertisement.  So instead create a
>>> "default
>>> route" flag in routes and create a new route with the local link prefix
>>> and the
>>> router source address.
>>>
>>
>> Note that RA does not always mean default route. You need to check
>> router lifetime to decide whether to use it as default route.
>
>
> I've been reading the spec a lot recently and I couldn't figure out a way to
> differentiate between a default route and just another route.

Probably I was not clear. You should not use RA to add *any* route
unless it claims to offer one. Your patch did not check for it.
"Default" was wrong here, I did not mean to distinguish between
different types of routes (actually, I am not sure if SLAAC can add
default route in usual sense).


>
> Eesh I just ripped that out didn't I?  Sorry about that, I'll rework this
> again.  I think I'll do like you said above, just kind of create an
> interface with different scopes and that way if you use both slaac and dhcp
> and get the same address we don't end up with two different interfaces.  And
> then I'll come up with something to tie the routers to the interfaces and
> rework the route stuff so it is a bit cleaner.  This will probably be
> towards the end of the week, I've found some weird timeout problem in the
> TCP stack I'm trying to track down, once I've figured that out I'll rework
> this patch.  Thanks,
>

Thank you for working on it!

[PATCH 2/3] net: add local route when creating link local ipv6 interface

[Josef Bacik]

In order to talk to link local ipv6 addresses we need to have a route out of the
link local interface for this to work.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 grub-core/net/net.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 21a4e94..f96297a 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -297,7 +297,9 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
   struct grub_net_network_level_interface *inf;
   char *name;
   char *ptr;
+  grub_err_t err;
   grub_net_network_level_address_t addr;
+  grub_net_network_level_netaddress_t netaddr;
 
   name = grub_malloc (grub_strlen (card->name)
 		      + GRUB_NET_MAX_STR_HWADDR_LEN
@@ -325,7 +327,17 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
       ptr += grub_strlen (ptr);
     }
   ptr = grub_stpcpy (ptr, ":link");
-  return grub_net_add_addr_real (name, card, &addr, hwaddr, 0);
+  inf = grub_net_add_addr_real (name, card, &addr, hwaddr, 0);
+  if (!inf)
+    return NULL;
+  netaddr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
+  netaddr.ipv6.base[0] = grub_cpu_to_be64_compile_time (0xfe80ULL << 48);
+  netaddr.ipv6.base[1] = 0;
+  netaddr.ipv6.masksize = 64;
+  err = grub_net_add_route (name, netaddr, inf);
+  if (err != GRUB_ERR_NONE)
+    return NULL;
+  return inf;
 }
 
 /* FIXME: allow to specify mac address.  */
-- 
1.8.5.6