Misc network boot patches

Misc network boot patches

[Matthew Garrett]

Various patches that we're using to support network boot in our setup. Only
number 3 is a bugfix.

[PATCH 1/4] Allow non-default ports for HTTP requests

[Matthew Garrett]

Add support for passing ports in HTTP requests. This takes the form of:
(http,serverip:portnum)/file
---
 grub-core/net/http.c |  8 ++++++--
 grub-core/net/net.c  | 10 +++++++++-
 include/grub/net.h   |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/grub-core/net/http.c b/grub-core/net/http.c
index 5aa4ad3..389a78e 100644
--- a/grub-core/net/http.c
+++ b/grub-core/net/http.c
@@ -309,7 +309,7 @@ http_establish (struct grub_file *file, grub_off_t offset, int initial)
 {
   http_data_t data = file->data;
   grub_uint8_t *ptr;
-  int i;
+  int i, port;
   struct grub_net_buff *nb;
   grub_err_t err;
 
@@ -390,8 +390,12 @@ http_establish (struct grub_file *file, grub_off_t offset, int initial)
   grub_netbuff_put (nb, 2);
   grub_memcpy (ptr, "\r\n", 2);
 
+  if (file->device->net->port)
+    port = file->device->net->port;
+  else
+    port = HTTP_PORT;
   data->sock = grub_net_tcp_open (file->device->net->server,
-				  HTTP_PORT, http_receive,
+				  port, http_receive,
 				  http_err, http_err,
 				  file);
   if (!data->sock)
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 10773fc..585f4f7 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -1261,7 +1261,7 @@ grub_net_open_real (const char *name)
   grub_net_app_level_t proto;
   const char *protname, *server;
   grub_size_t protnamelen;
-  int try;
+  int try, port = 0;
 
   if (grub_strncmp (name, "pxe:", sizeof ("pxe:") - 1) == 0)
     {
@@ -1278,7 +1278,14 @@ grub_net_open_real (const char *name)
   else
     {
       const char *comma;
+      char *colon;
       comma = grub_strchr (name, ',');
+      colon = grub_strchr (name, ':');
+      if (colon)
+	{
+	  port = (int) grub_strtol(colon+1, NULL, 10);
+	  *colon = '\0';
+	}
       if (comma)
 	{
 	  protnamelen = comma - name;
@@ -1310,6 +1317,7 @@ grub_net_open_real (const char *name)
 	    if (!ret)
 	      return NULL;
 	    ret->protocol = proto;
+	    ret->port = port;
 	    ret->server = grub_strdup (server);
 	    if (!ret->server)
 	      {
diff --git a/include/grub/net.h b/include/grub/net.h
index 2192fa1..c517509 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -276,6 +276,7 @@ typedef struct grub_net
   grub_fs_t fs;
   int eof;
   int stall;
+  int port;
 } *grub_net_t;
 
 extern grub_net_t (*EXPORT_VAR (grub_net_open)) (const char *name);
-- 
2.9.3

[PATCH 2/4] Send a user class identifier in bootp requests and tag it as DHCP discover

[Matthew Garrett]

It's helpful to determine that a request was sent by grub in order to permit
the server to provide different information at different stages of the boot
process. Send GRUB2 as a type 77 DHCP option when sending bootp packets in
order to make this possible and tag the request as a DHCP discover to
convince servers to pay attention to it. Add the client architecture for
good measure.
---
 grub-core/net/bootp.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 9e2fdb7..0da8e24 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -25,6 +25,24 @@
 #include <grub/net/udp.h>
 #include <grub/datetime.h>
 
+#if !defined(GRUB_MACHINE_EFI) && (defined(__i386__) || defined(__x86_64__))
+#define GRUB_NET_BOOTP_ARCH 0x0000
+#elif defined(GRUB_MACHINE_EFI) && defined(__x86_64__)
+#define GRUB_NET_BOOTP_ARCH 0x0007
+#elif defined(GRUB_MACHINE_EFI) && defined(__aarch64__)
+#define GRUB_NET_BOOTP_ARCH 0x000B
+#else
+#error "unknown bootp architecture"
+#endif
+
+static grub_uint8_t dhcp_option_header[] = {GRUB_NET_BOOTP_RFC1048_MAGIC_0,
+					    GRUB_NET_BOOTP_RFC1048_MAGIC_1,
+					    GRUB_NET_BOOTP_RFC1048_MAGIC_2,
+					    GRUB_NET_BOOTP_RFC1048_MAGIC_3};
+static grub_uint8_t grub_userclass[] = {0x4D, 0x06, 0x05, 'G', 'R', 'U', 'B', '2'};
+static grub_uint8_t grub_dhcpdiscover[] = {0x35, 0x01, 0x01};
+static grub_uint8_t grub_dhcptime[] = {0x33, 0x04, 0x00, 0x00, 0x0e, 0x10};
+
 static void
 parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
 {
@@ -499,10 +517,14 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
 	  struct udphdr *udph;
 	  grub_net_network_level_address_t target;
 	  grub_net_link_level_address_t ll_target;
+	  grub_uint8_t *offset;
 
 	  if (!ifaces[j].prev)
 	    continue;
-	  nb = grub_netbuff_alloc (sizeof (*pack) + 64 + 128);
+	  nb = grub_netbuff_alloc (sizeof (*pack) + sizeof(dhcp_option_header)
+				   + sizeof(grub_userclass)
+				   + sizeof(grub_dhcpdiscover)
+				   + sizeof(grub_dhcptime) + 64 + 128);
 	  if (!nb)
 	    {
 	      grub_netbuff_free (nb);
@@ -536,6 +558,24 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
 	  pack->seconds = grub_cpu_to_be16 (t);
 
 	  grub_memcpy (&pack->mac_addr, &ifaces[j].hwaddress.mac, 6); 
+	  offset = (grub_uint8_t *)&pack->vendor;
+	  grub_memcpy (offset, dhcp_option_header, sizeof(dhcp_option_header));
+	  offset += sizeof(dhcp_option_header);
+	  grub_memcpy (offset, grub_dhcpdiscover, sizeof(grub_dhcpdiscover));
+	  offset += sizeof(grub_dhcpdiscover);
+	  grub_memcpy (offset, grub_userclass, sizeof(grub_userclass));
+	  offset += sizeof(grub_userclass);
+	  grub_memcpy (offset, grub_dhcptime, sizeof(grub_dhcptime));
+
+	  /* insert Client System Architecture (option 93) */
+	  offset += sizeof(grub_dhcptime);
+	  offset[0] = 93;
+	  offset[1] = 2;
+	  offset[2] = (GRUB_NET_BOOTP_ARCH >> 8);
+	  offset[3] = (GRUB_NET_BOOTP_ARCH & 0xFF);
+
+	  /* option terminator */
+	  offset[4] = 255;
 
 	  grub_netbuff_push (nb, sizeof (*udph));
 
-- 
2.9.3

[PATCH 3/4] Don't allocate a new address buffer if we receive multiple DNS responses

[Matthew Garrett]

The current logic in the DNS resolution code allocates an address buffer
based on the number of addresses in the response packet. If we receive
multiple response packets in response to a single query packet, this means
that we will reallocate a new buffer large enough for only the addresses in
that specific packet, discarding any previous results in the process. Worse,
we still keep track of the *total* number of addresses resolved in response
to this query, not merely the number in the packet being currently processed.
Use realloc() rather than malloc() to avoid overwriting the existing data,
and allocate a buffer large enough for the total set of addresses rather
than merely the number in this specific response.
---
 grub-core/net/dns.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
index 5d9afe0..5deb1ef 100644
--- a/grub-core/net/dns.c
+++ b/grub-core/net/dns.c
@@ -285,8 +285,8 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
       ptr++;
       ptr += 4;
     }
-  *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
-				 * grub_be_to_cpu16 (head->ancount));
+  *data->addresses = grub_realloc (*data->addresses, sizeof ((*data->addresses)[0])
+		     * (grub_be_to_cpu16 (head->ancount) + *data->naddresses));
   if (!*data->addresses)
     {
       grub_errno = GRUB_ERR_NONE;
-- 
2.9.3

[PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

Some DHCP servers (such as dnsmasq) tokenise parameters with commas, making
it impossible to pass boot files with commas in them. Allow using a
semicolon to separate the protocol from host if a comma wasn't found.
---
 grub-core/net/net.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 585f4f7..efacb30 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -1280,6 +1280,10 @@ grub_net_open_real (const char *name)
       const char *comma;
       char *colon;
       comma = grub_strchr (name, ',');
+      if (!comma)
+	{
+	  comma = grub_strchr (name, ';');
+	}
       colon = grub_strchr (name, ':');
       if (colon)
 	{
-- 
2.9.3

Re: [PATCH 3/4] Don't allocate a new address buffer if we receive multiple DNS responses

[Andrei Borzenkov]

24.01.2017 03:36, Matthew Garrett пишет:
> The current logic in the DNS resolution code allocates an address buffer
> based on the number of addresses in the response packet. If we receive
> multiple response packets in response to a single query packet, this means
> that we will reallocate a new buffer large enough for only the addresses in
> that specific packet, discarding any previous results in the process. Worse,
> we still keep track of the *total* number of addresses resolved in response
> to this query, not merely the number in the packet being currently processed.
> Use realloc() rather than malloc() to avoid overwriting the existing data,
> and allocate a buffer large enough for the total set of addresses rather
> than merely the number in this specific response.
> ---
>  grub-core/net/dns.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
> index 5d9afe0..5deb1ef 100644
> --- a/grub-core/net/dns.c
> +++ b/grub-core/net/dns.c
> @@ -285,8 +285,8 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
>        ptr++;
>        ptr += 4;
>      }
> -  *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
> -				 * grub_be_to_cpu16 (head->ancount));
> +  *data->addresses = grub_realloc (*data->addresses, sizeof ((*data->addresses)[0])
> +		     * (grub_be_to_cpu16 (head->ancount) + *data->naddresses));

If *data->addresses was not NULL, we should not reach this code.

  /* Code apparently assumed that only one packet is received as response.
     We may get multiple responses due to network condition, so check here
     and quit early. */
  if (*data->addresses)
    {
      grub_netbuff_free (nb);
      return GRUB_ERR_NONE;
    }

This was noted previously by Josef, we discussed it and my position is
that resolver code requires redesign to correctly merge multiple answers
and prioritize A vs AAAA requests.

Do you get actual errors with current master? If yes, could you provide
more information what this patch fixes?

>    if (!*data->addresses)
>      {
>        grub_errno = GRUB_ERR_NONE;
> 

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Andrei Borzenkov]

24.01.2017 03:36, Matthew Garrett пишет:
> Some DHCP servers (such as dnsmasq) tokenise parameters with commas, making
> it impossible to pass boot files with commas in them. Allow using a

grub_net_open() operates on devices, not files. Please give more details
about your problem.

> semicolon to separate the protocol from host if a comma wasn't found.
> ---
>  grub-core/net/net.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 585f4f7..efacb30 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -1280,6 +1280,10 @@ grub_net_open_real (const char *name)
>        const char *comma;
>        char *colon;
>        comma = grub_strchr (name, ',');
> +      if (!comma)
> +	{
> +	  comma = grub_strchr (name, ';');
> +	}
>        colon = grub_strchr (name, ':');
>        if (colon)
>  	{
> 

Re: [PATCH 2/4] Send a user class identifier in bootp requests and tag it as DHCP discover

[Michael Marineau]

On Mon, Jan 23, 2017 at 4:35 PM, Matthew Garrett <mjg59@coreos.com> wrote:
> It's helpful to determine that a request was sent by grub in order to permit
> the server to provide different information at different stages of the boot
> process. Send GRUB2 as a type 77 DHCP option when sending bootp packets in
> order to make this possible and tag the request as a DHCP discover to
> convince servers to pay attention to it. Add the client architecture for
> good measure.
> ---
>  grub-core/net/bootp.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 9e2fdb7..0da8e24 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -25,6 +25,24 @@
>  #include <grub/net/udp.h>
>  #include <grub/datetime.h>
>
> +#if !defined(GRUB_MACHINE_EFI) && (defined(__i386__) || defined(__x86_64__))
> +#define GRUB_NET_BOOTP_ARCH 0x0000
> +#elif defined(GRUB_MACHINE_EFI) && defined(__x86_64__)
> +#define GRUB_NET_BOOTP_ARCH 0x0007
> +#elif defined(GRUB_MACHINE_EFI) && defined(__aarch64__)
> +#define GRUB_NET_BOOTP_ARCH 0x000B
> +#else
> +#error "unknown bootp architecture"
> +#endif

This list is woefully incomplete and there aren't even allocated
values for every platform that grub supports so adding the
architecture option will need to be optional.

> +
> +static grub_uint8_t dhcp_option_header[] = {GRUB_NET_BOOTP_RFC1048_MAGIC_0,
> +                                           GRUB_NET_BOOTP_RFC1048_MAGIC_1,
> +                                           GRUB_NET_BOOTP_RFC1048_MAGIC_2,
> +                                           GRUB_NET_BOOTP_RFC1048_MAGIC_3};
> +static grub_uint8_t grub_userclass[] = {0x4D, 0x06, 0x05, 'G', 'R', 'U', 'B', '2'};
> +static grub_uint8_t grub_dhcpdiscover[] = {0x35, 0x01, 0x01};
> +static grub_uint8_t grub_dhcptime[] = {0x33, 0x04, 0x00, 0x00, 0x0e, 0x10};
> +
>  static void
>  parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
>  {
> @@ -499,10 +517,14 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
>           struct udphdr *udph;
>           grub_net_network_level_address_t target;
>           grub_net_link_level_address_t ll_target;
> +         grub_uint8_t *offset;
>
>           if (!ifaces[j].prev)
>             continue;
> -         nb = grub_netbuff_alloc (sizeof (*pack) + 64 + 128);
> +         nb = grub_netbuff_alloc (sizeof (*pack) + sizeof(dhcp_option_header)
> +                                  + sizeof(grub_userclass)
> +                                  + sizeof(grub_dhcpdiscover)
> +                                  + sizeof(grub_dhcptime) + 64 + 128);

Increasing the allocation without also updating grub_netbuff_reserve
and grub_netbuff_push calls doesn't actually influence the amount of
space in the packet. grub_netbuff_alloc and grub_netbuff_reserve
should be given the total size of the packet: ethernet+ip+udp+bootp
while grub_netbuff_push is called for each of those layers. As-is this
happens to work because the existing code included 64 bytes of zeros
at the end of the bootp packet. The new options use 26 of those bytes.
The extra 128 bytes of the reservation is for the lower protocol
headers (ethernet+ip+udp).

I don't now why the original code included 64 bytes of trailing zeros
or allocates 128 for the lower protocols when only 42 will ever be
used I don't know, that seems to be the state of the code on initial
check-in to git.

>           if (!nb)
>             {
>               grub_netbuff_free (nb);
> @@ -536,6 +558,24 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
>           pack->seconds = grub_cpu_to_be16 (t);
>
>           grub_memcpy (&pack->mac_addr, &ifaces[j].hwaddress.mac, 6);
> +         offset = (grub_uint8_t *)&pack->vendor;
> +         grub_memcpy (offset, dhcp_option_header, sizeof(dhcp_option_header));
> +         offset += sizeof(dhcp_option_header);
> +         grub_memcpy (offset, grub_dhcpdiscover, sizeof(grub_dhcpdiscover));
> +         offset += sizeof(grub_dhcpdiscover);
> +         grub_memcpy (offset, grub_userclass, sizeof(grub_userclass));
> +         offset += sizeof(grub_userclass);
> +         grub_memcpy (offset, grub_dhcptime, sizeof(grub_dhcptime));
> +
> +         /* insert Client System Architecture (option 93) */
> +         offset += sizeof(grub_dhcptime);
> +         offset[0] = 93;
> +         offset[1] = 2;
> +         offset[2] = (GRUB_NET_BOOTP_ARCH >> 8);
> +         offset[3] = (GRUB_NET_BOOTP_ARCH & 0xFF);
> +
> +         /* option terminator */
> +         offset[4] = 255;
>
>           grub_netbuff_push (nb, sizeof (*udph));
>
> --
> 2.9.3
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

On Mon, Jan 23, 2017 at 8:02 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 24.01.2017 03:36, Matthew Garrett пишет:
>> Some DHCP servers (such as dnsmasq) tokenise parameters with commas, making
>> it impossible to pass boot files with commas in them. Allow using a
>
> grub_net_open() operates on devices, not files. Please give more details
> about your problem.

The DHCP server will return a string in the boot_file field. If you
want to indicate that this file should be obtained over http, the
easiest way to handle this is to provide a boot file in the form
(http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
configuration parameters and there appears to be no way to override
that, which makes it impossible to provide a boot file in this form.
Allowing the use of an alternative character avoids this problem.

Re: [PATCH 3/4] Don't allocate a new address buffer if we receive multiple DNS responses

[Matthew Garrett]

On Mon, Jan 23, 2017 at 7:55 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> This was noted previously by Josef, we discussed it and my position is
> that resolver code requires redesign to correctly merge multiple answers
> and prioritize A vs AAAA requests.
>
> Do you get actual errors with current master? If yes, could you provide
> more information what this patch fixes?

Ah, yeah, it looks like this is unnecessary with the current state of
the code. I'll drop it, thanks!

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Andrei Borzenkov]

24.01.2017 23:50, Matthew Garrett пишет:
> On Mon, Jan 23, 2017 at 8:02 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>> 24.01.2017 03:36, Matthew Garrett пишет:
>>> Some DHCP servers (such as dnsmasq) tokenise parameters with commas, making
>>> it impossible to pass boot files with commas in them. Allow using a
>>
>> grub_net_open() operates on devices, not files. Please give more details
>> about your problem.
> 
> The DHCP server will return a string in the boot_file field. If you
> want to indicate that this file should be obtained over http, the
> easiest way to handle this is to provide a boot file in the form
> (http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
> configuration parameters and there appears to be no way to override
> that, which makes it impossible to provide a boot file in this form.
> Allowing the use of an alternative character avoids this problem.
> 

This won't work because (http,host) will never be interpreted as device
in current code so you need to support this first before this patch can
even be considered. Also I am not convinced that arbitrary changing
syntax is good idea.

You already can set $prefix when generating image. Why is it not enough?

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

On Tue, Jan 24, 2017 at 7:48 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 24.01.2017 23:50, Matthew Garrett пишет:
>> On Mon, Jan 23, 2017 at 8:02 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>>> 24.01.2017 03:36, Matthew Garrett пишет:
>>>> Some DHCP servers (such as dnsmasq) tokenise parameters with commas, making
>>>> it impossible to pass boot files with commas in them. Allow using a
>>>
>>> grub_net_open() operates on devices, not files. Please give more details
>>> about your problem.
>>
>> The DHCP server will return a string in the boot_file field. If you
>> want to indicate that this file should be obtained over http, the
>> easiest way to handle this is to provide a boot file in the form
>> (http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
>> configuration parameters and there appears to be no way to override
>> that, which makes it impossible to provide a boot file in this form.
>> Allowing the use of an alternative character avoids this problem.
>>
>
> This won't work because (http,host) will never be interpreted as device
> in current code so you need to support this first before this patch can
> even be considered. Also I am not convinced that arbitrary changing
> syntax is good idea.

I don't understand - grub_net_open_real() already handles this case:

  else
    {
      const char *comma;
      comma = grub_strchr (name, ',');
      if (comma)
        {
          protnamelen = comma - name;
          server = comma + 1;
          protname = name;
        }
      else
        {
          protnamelen = grub_strlen (name);
          server = grub_net_default_server;
          protname = name;
        }


> You already can set $prefix when generating image. Why is it not enough?

Because we ship prebuilt images but don't know what IP addresses users
will be using for their deployment servers.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Andrei Borzenkov]

25.01.2017 07:06, Matthew Garrett пишет:
> On Tue, Jan 24, 2017 at 7:48 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>> 24.01.2017 23:50, Matthew Garrett пишет:
>>> On Mon, Jan 23, 2017 at 8:02 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>>>> 24.01.2017 03:36, Matthew Garrett пишет:
>>>>> Some DHCP servers (such as dnsmasq) tokenise parameters with commas, making
>>>>> it impossible to pass boot files with commas in them. Allow using a
>>>>
>>>> grub_net_open() operates on devices, not files. Please give more details
>>>> about your problem.
>>>
>>> The DHCP server will return a string in the boot_file field. If you
>>> want to indicate that this file should be obtained over http, the
>>> easiest way to handle this is to provide a boot file in the form
>>> (http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
>>> configuration parameters and there appears to be no way to override
>>> that, which makes it impossible to provide a boot file in this form.
>>> Allowing the use of an alternative character avoids this problem.
>>>
>>
>> This won't work because (http,host) will never be interpreted as device
>> in current code so you need to support this first before this patch can
>> even be considered. Also I am not convinced that arbitrary changing
>> syntax is good idea.
> 
> I don't understand - grub_net_open_real() already handles this case:

Because bootfile from DHCP packet is not used to set device part of
$prefix. Setting bootfile to (http,host)filename will end up with full
prefix "(tftp,$next_ip)(http,host)base-name-of-filename". If you mean
something different, please explain.

> 
>   else
>     {
>       const char *comma;
>       comma = grub_strchr (name, ',');
>       if (comma)
>         {
>           protnamelen = comma - name;
>           server = comma + 1;
>           protname = name;
>         }
>       else
>         {
>           protnamelen = grub_strlen (name);
>           server = grub_net_default_server;
>           protname = name;
>         }
> 
> 
>> You already can set $prefix when generating image. Why is it not enough?
> 
> Because we ship prebuilt images but don't know what IP addresses users
> will be using for their deployment servers.
> 

I can think about implementing URI parsing (somewhat in line with UEFI
HTTPboot spec) and/or supporting partial $prefix that sets only
protocol, something like "grub-mknetdir --prefix=(http,)", where server
part is filled from DHCP ACK.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

On Tue, Jan 24, 2017 at 8:15 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 25.01.2017 07:06, Matthew Garrett пишет:
>> I don't understand - grub_net_open_real() already handles this case:
>
> Because bootfile from DHCP packet is not used to set device part of
> $prefix. Setting bootfile to (http,host)filename will end up with full
> prefix "(tftp,$next_ip)(http,host)base-name-of-filename". If you mean
> something different, please explain.

If prefix isn't set then won't bootfile be interpreted as the device plus file?

>> Because we ship prebuilt images but don't know what IP addresses users
>> will be using for their deployment servers.
>>
>
> I can think about implementing URI parsing (somewhat in line with UEFI
> HTTPboot spec) and/or supporting partial $prefix that sets only
> protocol, something like "grub-mknetdir --prefix=(http,)", where server
> part is filled from DHCP ACK.

We need the ability to pass port as well, so that would still be insufficient.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Michael Chang]

On Tue, Jan 24, 2017 at 12:50:37PM -0800, Matthew Garrett wrote:
> On Mon, Jan 23, 2017 at 8:02 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> > 24.01.2017 03:36, Matthew Garrett пишет:
> >> Some DHCP servers (such as dnsmasq) tokenise parameters with commas, making
> >> it impossible to pass boot files with commas in them. Allow using a
> >
> > grub_net_open() operates on devices, not files. Please give more details
> > about your problem.
> 
> The DHCP server will return a string in the boot_file field. If you
> want to indicate that this file should be obtained over http, the
> easiest way to handle this is to provide a boot file in the form
> (http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
> configuration parameters and there appears to be no way to override
> that, which makes it impossible to provide a boot file in this form.
> Allowing the use of an alternative character avoids this problem.

To my understanding, you can (and have to) use the url form in boot-file and
specify vendor-class-identifier to "HTTPClient" to let the UEFI firmware knows
this as a response to HTTP boot request and handle that boot-file accordingly,
then continue with that url loaded and run as NBP.

Here the article provide some examples.

https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF

I suppose you may be running the bootp for setting up the client network
interface and wanted it to have work with http hosting the os images in some
way ? Anyway the config seems not to work with what UEFI prosposed for HTTP
Boot and I'd suggest to check that first.

Thanks,
Michael

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

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

On Tue, Jan 24, 2017 at 10:18 PM, Michael Chang <mchang@suse.com> wrote:
> On Tue, Jan 24, 2017 at 12:50:37PM -0800, Matthew Garrett wrote:
>> The DHCP server will return a string in the boot_file field. If you
>> want to indicate that this file should be obtained over http, the
>> easiest way to handle this is to provide a boot file in the form
>> (http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
>> configuration parameters and there appears to be no way to override
>> that, which makes it impossible to provide a boot file in this form.
>> Allowing the use of an alternative character avoids this problem.
>
> To my understanding, you can (and have to) use the url form in boot-file and
> specify vendor-class-identifier to "HTTPClient" to let the UEFI firmware knows
> this as a response to HTTP boot request and handle that boot-file accordingly,
> then continue with that url loaded and run as NBP.

We're passing the bootfile to grub in order to obtain further
configuration, so the firmware isn't relevant here.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Andrei Borzenkov]

On Wed, Jan 25, 2017 at 7:25 AM, Matthew Garrett <mjg59@coreos.com> wrote:
> On Tue, Jan 24, 2017 at 8:15 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>> 25.01.2017 07:06, Matthew Garrett пишет:
>>> I don't understand - grub_net_open_real() already handles this case:
>>
>> Because bootfile from DHCP packet is not used to set device part of
>> $prefix. Setting bootfile to (http,host)filename will end up with full
>> prefix "(tftp,$next_ip)(http,host)base-name-of-filename". If you mean
>> something different, please explain.
>
> If prefix isn't set then won't bootfile be interpreted as the device plus file?
>

No. That would mean "parsing URI" that I mentioned.

>>> Because we ship prebuilt images but don't know what IP addresses users
>>> will be using for their deployment servers.
>>>
>>
>> I can think about implementing URI parsing (somewhat in line with UEFI
>> HTTPboot spec) and/or supporting partial $prefix that sets only
>> protocol, something like "grub-mknetdir --prefix=(http,)", where server
>> part is filled from DHCP ACK.
>
> We need the ability to pass port as well, so that would still be insufficient.

Good. So start with design proposal that is extensible enough.

But TBH - all of this can already be implemented using grub scripting.
Use custom DHCP options or parse bootfile using regex. No code changes
is needed - we support generic scripting for a reason.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

On Tue, Jan 24, 2017 at 10:56 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> On Wed, Jan 25, 2017 at 7:25 AM, Matthew Garrett <mjg59@coreos.com> wrote:
>> If prefix isn't set then won't bootfile be interpreted as the device plus file?
>>
>
> No. That would mean "parsing URI" that I mentioned.

My experience is that configfile (http,example.com)grub/config works
as you'd expect it to, and that set
endpoint=$net_efinet0_dhcp_boot_file; configfile $endpoint does the
same. Am I hitting some corner case where things are being incorrectly
parsed and so giving me unintended functionality?

>> We need the ability to pass port as well, so that would still be insufficient.
>
> Good. So start with design proposal that is extensible enough.
>
> But TBH - all of this can already be implemented using grub scripting.
> Use custom DHCP options or parse bootfile using regex. No code changes
> is needed - we support generic scripting for a reason.

How are custom DHCP options handled? I can't find anything in the
documentation about them being interpreted, and a quick look at the
code only shows it setting known variables.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Andrei Borzenkov]

On Wed, Jan 25, 2017 at 10:16 AM, Matthew Garrett <mjg59@coreos.com> wrote:
> On Tue, Jan 24, 2017 at 10:56 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>> On Wed, Jan 25, 2017 at 7:25 AM, Matthew Garrett <mjg59@coreos.com> wrote:
>>> If prefix isn't set then won't bootfile be interpreted as the device plus file?
>>>
>>
>> No. That would mean "parsing URI" that I mentioned.
>
> My experience is that configfile (http,example.com)grub/config works
> as you'd expect it to, and that set
> endpoint=$net_efinet0_dhcp_boot_file; configfile $endpoint does the
> same. Am I hitting some corner case where things are being incorrectly
> parsed and so giving me unintended functionality?
>

When grub starts it tries to determine device and path it was booted
from. For network boot it currently means examining DHCP ACK packet
that is normally provided by firmware and setting device to
tftp,$next_ip and path to $bootfile. There is no provision to set
protocol to anything else nor to parse $bootfile to extract
protocol/server.

If you speak about "configfile something" you are past this point so
DHCP $bootfile option is not relevant at all.

>>> We need the ability to pass port as well, so that would still be insufficient.
>>
>> Good. So start with design proposal that is extensible enough.
>>
>> But TBH - all of this can already be implemented using grub scripting.
>> Use custom DHCP options or parse bootfile using regex. No code changes
>> is needed - we support generic scripting for a reason.
>
> How are custom DHCP options handled? I can't find anything in the
> documentation about them being interpreted, and a quick look at the
> code only shows it setting known variables.

You have net_get_dhcp_option to fetch arbitrary option value from DHCP
ACK packet and put it in variable for further processing. I'm
definitely open to improving this command to make it more useful if it
turns out lacking something.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Michael Chang]

On Tue, Jan 24, 2017 at 10:21:22PM -0800, Matthew Garrett wrote:
> On Tue, Jan 24, 2017 at 10:18 PM, Michael Chang <mchang@suse.com> wrote:
> > On Tue, Jan 24, 2017 at 12:50:37PM -0800, Matthew Garrett wrote:
> >> The DHCP server will return a string in the boot_file field. If you
> >> want to indicate that this file should be obtained over http, the
> >> easiest way to handle this is to provide a boot file in the form
> >> (http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
> >> configuration parameters and there appears to be no way to override
> >> that, which makes it impossible to provide a boot file in this form.
> >> Allowing the use of an alternative character avoids this problem.
> >
> > To my understanding, you can (and have to) use the url form in boot-file and
> > specify vendor-class-identifier to "HTTPClient" to let the UEFI firmware knows
> > this as a response to HTTP boot request and handle that boot-file accordingly,
> > then continue with that url loaded and run as NBP.
> 
> We're passing the bootfile to grub in order to obtain further
> configuration, so the firmware isn't relevant here.

I mean that firmware may not be involved for now but it may be helpful someday
to carry that very same config booted by firmware as is.

And firmware did have implications to $prefix, if it's not been set, the value
would be the booted device and path pass by firmware. To keep from ABI
conflicts in modules loaded from elsewhere, you should not change it (in that
further configration) unless you know the modules will always be compatible in
new $prefix location.

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

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

On Tue, Jan 24, 2017 at 11:37 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> On Wed, Jan 25, 2017 at 10:16 AM, Matthew Garrett <mjg59@coreos.com> wrote:
>> My experience is that configfile (http,example.com)grub/config works
>> as you'd expect it to, and that set
>> endpoint=$net_efinet0_dhcp_boot_file; configfile $endpoint does the
>> same. Am I hitting some corner case where things are being incorrectly
>> parsed and so giving me unintended functionality?
>>
>
> When grub starts it tries to determine device and path it was booted
> from. For network boot it currently means examining DHCP ACK packet
> that is normally provided by firmware and setting device to
> tftp,$next_ip and path to $bootfile. There is no provision to set
> protocol to anything else nor to parse $bootfile to extract
> protocol/server.
>
> If you speak about "configfile something" you are past this point so
> DHCP $bootfile option is not relevant at all.

The variable is accessible to any configuration file that was built
into the grub binary, so it's still very relevant. Our flow is to have
the firmware DHCP and obtain grub, then have grub send a new DHCP
request with a different user agent (see previous patch) and obtain a
new bootfile.

>> How are custom DHCP options handled? I can't find anything in the
>> documentation about them being interpreted, and a quick look at the
>> code only shows it setting known variables.
>
> You have net_get_dhcp_option to fetch arbitrary option value from DHCP
> ACK packet and put it in variable for further processing. I'm
> definitely open to improving this command to make it more useful if it
> turns out lacking something.

Hm. In theory sticking everything in option 43 and parsing it out in
the config is possible - in practice the only reason that would be
necessary right now is that grub uses the same separator as dnsmasq,
and a trivial patch to grub would make it much easier to provide
configuration on the dnsmasq command line.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

On Wed, Jan 25, 2017 at 12:35 AM, Michael Chang <mchang@suse.com> wrote:
> On Tue, Jan 24, 2017 at 10:21:22PM -0800, Matthew Garrett wrote:
>> We're passing the bootfile to grub in order to obtain further
>> configuration, so the firmware isn't relevant here.
>
> I mean that firmware may not be involved for now but it may be helpful someday
> to carry that very same config booted by firmware as is.

The information is there. If local configuration doesn't choose to do
something that overrides it, it will stay there.

> And firmware did have implications to $prefix, if it's not been set, the value
> would be the booted device and path pass by firmware. To keep from ABI
> conflicts in modules loaded from elsewhere, you should not change it (in that
> further configration) unless you know the modules will always be compatible in
> new $prefix location.

The DHCP server provided the initial copy of grub, so it should ensure
that it only provides links to configuration that will provide
compatible modules. This is no worse than supporting reading config
fragments off disk that might redefine prefix.

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Andrei Borzenkov]

24.01.2017 23:50, Matthew Garrett пишет:
> On Mon, Jan 23, 2017 at 8:02 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>> 24.01.2017 03:36, Matthew Garrett пишет:
>>> Some DHCP servers (such as dnsmasq) tokenise parameters with commas, making
>>> it impossible to pass boot files with commas in them. Allow using a
>>
>> grub_net_open() operates on devices, not files. Please give more details
>> about your problem.
> 
> The DHCP server will return a string in the boot_file field. If you
> want to indicate that this file should be obtained over http, the
> easiest way to handle this is to provide a boot file in the form
> (http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
> configuration parameters and there appears to be no way to override
> that, which makes it impossible to provide a boot file in this form.

Really?

dnsmasq -d -z --dhcp-range=192.168.11.20,192.168.11.30 --dhcp-option
option:bootfile-name,'(http,1.2.3.4)/foo/bar' -i eth1


Bootstrap Protocol (ACK)
...
    Option: (67) Bootfile name
        Length: 23
        Bootfile name: (http,1.2.3.4)/foo/bar
    Option: (255) End
        Option End: 255

Re: [PATCH 4/4] Allow protocol to be separated from host with a semicolon

[Matthew Garrett]

On Wed, Jan 25, 2017 at 9:30 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 24.01.2017 23:50, Matthew Garrett пишет:
>> The DHCP server will return a string in the boot_file field. If you
>> want to indicate that this file should be obtained over http, the
>> easiest way to handle this is to provide a boot file in the form
>> (http,host)filename. Unfortunately dnsmasq uses commas to tokenise its
>> configuration parameters and there appears to be no way to override
>> that, which makes it impossible to provide a boot file in this form.
>
> Really?
>
> dnsmasq -d -z --dhcp-range=192.168.11.20,192.168.11.30 --dhcp-option
> option:bootfile-name,'(http,1.2.3.4)/foo/bar' -i eth1
>
>
> Bootstrap Protocol (ACK)
> ...
>     Option: (67) Bootfile name
>         Length: 23
>         Bootfile name: (http,1.2.3.4)/foo/bar
>     Option: (255) End
>         Option End: 255

Huh. Apparently I was very confused. Sorry about the waste of time!

Re: [PATCH 1/4] Allow non-default ports for HTTP requests

[Andrei Borzenkov]

24.01.2017 03:35, Matthew Garrett пишет:
> Add support for passing ports in HTTP requests. This takes the form of:
> (http,serverip:portnum)/file
> ---
>  grub-core/net/http.c |  8 ++++++--
>  grub-core/net/net.c  | 10 +++++++++-
>  include/grub/net.h   |  1 +
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index 5aa4ad3..389a78e 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -309,7 +309,7 @@ http_establish (struct grub_file *file, grub_off_t offset, int initial)
>  {
>    http_data_t data = file->data;
>    grub_uint8_t *ptr;
> -  int i;
> +  int i, port;
>    struct grub_net_buff *nb;
>    grub_err_t err;
>  
> @@ -390,8 +390,12 @@ http_establish (struct grub_file *file, grub_off_t offset, int initial)
>    grub_netbuff_put (nb, 2);
>    grub_memcpy (ptr, "\r\n", 2);
>  
> +  if (file->device->net->port)

0 is valid port number (at least I am not aware of any RFC that
prohibits its use).

> +    port = file->device->net->port;
> +  else
> +    port = HTTP_PORT;
>    data->sock = grub_net_tcp_open (file->device->net->server,
> -				  HTTP_PORT, http_receive,
> +				  port, http_receive,
>  				  http_err, http_err,
>  				  file);
>    if (!data->sock)
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 10773fc..585f4f7 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -1261,7 +1261,7 @@ grub_net_open_real (const char *name)
>    grub_net_app_level_t proto;
>    const char *protname, *server;
>    grub_size_t protnamelen;
> -  int try;
> +  int try, port = 0;
>  
>    if (grub_strncmp (name, "pxe:", sizeof ("pxe:") - 1) == 0)
>      {
> @@ -1278,7 +1278,14 @@ grub_net_open_real (const char *name)
>    else
>      {
>        const char *comma;
> +      char *colon;
>        comma = grub_strchr (name, ',');
> +      colon = grub_strchr (name, ':');

This conflicts with IPv6 addresses. Similar patches were already posted.
I suggested using (proto,server,port). This also allows empty server and
still using non-standard port.

More general at some point we should hit authentication, and so may need
to stuff user/password there. So we need to come up with extensible
framework.

Putting everything in "server" string may work, but then we need to
support IPv6 literals here - [fe80::1]:444 - at least, this is the only
way to use server:port I see. Also, we probably need to support escaping
of colons. Finally, speaking about user/password, we may need to support

user:password@[fe80::1]:456

May be using (net,proto=http,server=xxx,port=yyy,user=uuu,...) which
allows adding arbitrary keywords at any time.

[PATCH 3/4] Don't allocate a new address buffer if we receive multiple DNS responses

[Matthew Garrett]

The current logic in the DNS resolution code allocates an address buffer
based on the number of addresses in the response packet. If we receive
multiple response packets in response to a single query packet, this means
that we will reallocate a new buffer large enough for only the addresses in
that specific packet, discarding any previous results in the process. Worse,
we still keep track of the *total* number of addresses resolved in response
to this query, not merely the number in the packet being currently processed.
Use realloc() rather than malloc() to avoid overwriting the existing data,
and allocate a buffer large enough for the total set of addresses rather
than merely the number in this specific response.
---
 grub-core/net/dns.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
index 5d9afe0..5deb1ef 100644
--- a/grub-core/net/dns.c
+++ b/grub-core/net/dns.c
@@ -285,8 +285,8 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
       ptr++;
       ptr += 4;
     }
-  *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
-				 * grub_be_to_cpu16 (head->ancount));
+  *data->addresses = grub_realloc (*data->addresses, sizeof ((*data->addresses)[0])
+		     * (grub_be_to_cpu16 (head->ancount) + *data->naddresses));
   if (!*data->addresses)
     {
       grub_errno = GRUB_ERR_NONE;
-- 
2.9.3