* [PATCH v3 00/10] net: bootp: add native DHCPv4 support
@ 2019-03-07 15:14 Andre Przywara
2019-03-07 15:14 ` [PATCH v3 01/10] net: dhcp: remove dead code Andre Przywara
` (10 more replies)
0 siblings, 11 replies; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
A minor rework compared to v2, addressing Daniel's comments on the list.
The former patch 2/9 has been split up to first refactor the
parse_dhcp_vendor() function, then later introduce the OVERLOAD
functionality. The other minor nits should be fixed as well, also I
added Daniel's Reviewed-by: tags.
Again tested against a modern DHCP-only server and some legacy bootpd
version.
Cheers,
Andre.
Original commit message:
----------------------------------
This patch adds support for native DHCPv4 and removes requirement for
BOOTP compatibility support in DHCP server.
There is no provision for selecting preferred server. We take the first
OFFER and try to REQUEST configuration from it. If NAK was received,
transaction is restarted, but if we hit timeout, configuration fails.
It also handles pure BOOTP reply (detected by lack of DHCP message type
option) and proceeds with configuration immedately. I could not test it
because I do not have pure BOOTP server.
Because we need access to DHCP options in multiple places now, it also
factors out DHCP option processing, adding support for option overload.
Timeout handling is now per-interface, with independent timeouts for
both DISCOVER and REQUEST. It should make it more responsive if answer
was delayed initially. Total timeout remains the same as originally, as
well as backoff algorithm, but we poll in fixed 200ms ticks so notice
(successful) reply earlier.
Failure to send packet to interface now does not terminate command (and
leaks memory) but rather simply ignores this interface and continues with
remaining ones if present.
Finally it adds net_dhcp alias with intention to deprecate net_bootp
completely (it would be possible to make net_bootp to actually send BOOTP
packet if someone thinks it is required).
Features not implements:
- DHCP server selection. We take first DHCPOFFER or BOOTPREPLY. No plans
to implement.
- DHCP option concatenation (RFC3396). I do not expect to hit it in real
life and it rather complicates implementation, so let's wait for actual
use case.
- client identifier (RFC6842). So far we expect valid MAC address, which
should be enough to uniquely identify client. It is also not clear how to
generate unique client identifier if we ever need one.
v2: change find_dhcp_option to use subscripts to avoid signed/unsigned
comparison warning.
Should fix "may be used uninitialized" warning (although I could not
reproduce it).
---------
This is a rebased version of Andrei's work from 2016 [1][2][3].
We have still this issue here where our company DHCP server does not
support the BOOTP protocol, so grub doesn't get a valid IP address.
I took v2 from Andrei of the list, and fixed the minor conflicts during the
rebase. I can confirm that this patch makes the difference between DHCP
working and not:
grub-master> net_bootp
error: couldn't autoconfigure efinet0.
...
grub-patched> net_bootp
grub-patched> net_ls_addr
efinet0:dhcp 00:11:22:33:44:55 10.1.23.42
Also back in the days there were concerns about regressing BOOTP-only
servers. So I took the CMU bootpd[4], disabled the DHCP extensions at
compile time and still got IP addresses in both cases (patched/unpatched).
dhclient on Linux on the other hand was not happy with that server and
ignored the reply. I guess this is as close to a "BOOTP only server" as
we can get in 2019.
I would be very happy if we can get this merged now.
Please let me know if you need more information or any help on this.
Thanks!
Andre.
P.S. The original patch didn't have a Signed-off-by:, so I kept it that
way. Added mine for legal reason, feel free to drop it.
[1] http://lists.gnu.org/archive/html/grub-devel/2016-01/msg00035.html
[2] http://lists.gnu.org/archive/html/grub-devel/2016-03/msg00276.html
[3] http://lists.gnu.org/archive/html/grub-devel/2016-04/msg00066.html
[4] https://packages.debian.org/jessie/bootp
Andrei Borzenkov (10):
net: dhcp: remove dead code
net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option()
net: dhcp: Allow overloading legacy bootfile and name field
net: dhcp: refactor DHCP packet transmission into separate function
net: dhcp: make grub_net_process_dhcp take an interface
net: dhcp: introduce per-interface timeout
net: dhcp: use DHCP options for name and bootfile
net: dhcp: allow receiving DHCP OFFER and ACK packets
net: dhcp: actually send out DHCPv4 DISCOVER and REQUEST messages
net: dhcp: add explicit net_dhcp command
grub-core/net/bootp.c | 772 +++++++++++++++++++++++++++++-------------
grub-core/net/ip.c | 2 +-
include/grub/net.h | 14 +-
3 files changed, 543 insertions(+), 245 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 01/10] net: dhcp: remove dead code
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-07 15:14 ` [PATCH v3 02/10] net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option() Andre Przywara
` (9 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
The comment is right, the "giaddr" fields holds the IP address of the
BOOTP relay, not a general purpose router address.
Just remove the commented code, archeologists can find it in the git
history.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/net/bootp.c | 29 -----------------------------
1 file changed, 29 deletions(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 9e2fdb795..c92dfbd3a 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -161,35 +161,6 @@ grub_net_configure_by_dhcp_ack (const char *name,
if (!inter)
return 0;
-#if 0
- /* This is likely based on misunderstanding. gateway_ip refers to
- address of BOOTP relay and should not be used after BOOTP transaction
- is complete.
- See RFC1542, 3.4 Interpretation of the 'giaddr' field
- */
- if (bp->gateway_ip)
- {
- grub_net_network_level_netaddress_t target;
- grub_net_network_level_address_t gw;
- char *rname;
-
- target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
- target.ipv4.base = bp->server_ip;
- target.ipv4.masksize = 32;
- gw.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
- gw.ipv4 = bp->gateway_ip;
- rname = grub_xasprintf ("%s:gw", name);
- if (rname)
- grub_net_add_route_gw (rname, target, gw);
- grub_free (rname);
-
- target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
- target.ipv4.base = bp->gateway_ip;
- target.ipv4.masksize = 32;
- grub_net_add_route (name, target, inter);
- }
-#endif
-
if (size > OFFSET_OF (boot_file, bp))
grub_env_set_net_property (name, "boot_file", bp->boot_file,
sizeof (bp->boot_file));
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 02/10] net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option()
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
2019-03-07 15:14 ` [PATCH v3 01/10] net: dhcp: remove dead code Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-08 11:29 ` Daniel Kiper
2019-03-07 15:14 ` [PATCH v3 03/10] net: dhcp: Allow overloading legacy bootfile and name field Andre Przywara
` (8 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
For proper DHCP support we will need to parse DHCP options from a packet
more often and at various places.
Refactor the option parsing into a new function, which will scan a
packet to find *a particular* option field.
Use that new function in places where we were dealing with DHCP options
before.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
grub-core/net/bootp.c | 227 ++++++++++++++++++++++--------------------
1 file changed, 117 insertions(+), 110 deletions(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index c92dfbd3a..8d6763689 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -25,25 +25,41 @@
#include <grub/net/udp.h>
#include <grub/datetime.h>
-static void
-parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
+static const void *
+find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
+ grub_uint8_t opt_code, grub_uint8_t *opt_len)
{
- const grub_uint8_t *ptr, *ptr0;
+ const grub_uint8_t *ptr;
+ grub_size_t i;
- ptr = ptr0 = vend;
+ if (opt_len)
+ *opt_len = 0;
+
+ /* Is the packet big enough to hold at least the magic cookie? */
+ if (size < sizeof (*bp) + sizeof (grub_uint32_t))
+ return NULL;
+
+ /*
+ * Pointer arithmetic to point behind the common stub packet, where
+ * the options start.
+ */
+ ptr = (grub_uint8_t *) (bp + 1);
if (ptr[0] != GRUB_NET_BOOTP_RFC1048_MAGIC_0
|| ptr[1] != GRUB_NET_BOOTP_RFC1048_MAGIC_1
|| ptr[2] != GRUB_NET_BOOTP_RFC1048_MAGIC_2
|| ptr[3] != GRUB_NET_BOOTP_RFC1048_MAGIC_3)
- return;
- ptr = ptr + sizeof (grub_uint32_t);
- while (ptr - ptr0 < limit)
+ return NULL;
+
+ size -= sizeof (*bp);
+ i = sizeof (grub_uint32_t);
+
+ while (i < size)
{
grub_uint8_t tagtype;
grub_uint8_t taglength;
- tagtype = *ptr++;
+ tagtype = ptr[i++];
/* Pad tag. */
if (tagtype == GRUB_NET_BOOTP_PAD)
@@ -51,81 +67,27 @@ parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
/* End tag. */
if (tagtype == GRUB_NET_BOOTP_END)
- return;
+ break;
- taglength = *ptr++;
+ if (i >= size)
+ return NULL;
- switch (tagtype)
- {
- case GRUB_NET_BOOTP_NETMASK:
- if (taglength == 4)
- {
- int i;
- for (i = 0; i < 32; i++)
- if (!(ptr[i / 8] & (1 << (7 - (i % 8)))))
- break;
- *mask = i;
- }
- break;
+ taglength = ptr[i++];
+ if (i + taglength >= size)
+ return NULL;
- case GRUB_NET_BOOTP_ROUTER:
- if (taglength == 4)
- {
- grub_net_network_level_netaddress_t target;
- grub_net_network_level_address_t gw;
- char *rname;
-
- target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
- target.ipv4.base = 0;
- target.ipv4.masksize = 0;
- gw.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
- grub_memcpy (&gw.ipv4, ptr, sizeof (gw.ipv4));
- rname = grub_xasprintf ("%s:default", name);
- if (rname)
- grub_net_add_route_gw (rname, target, gw, NULL);
- grub_free (rname);
- }
- break;
- case GRUB_NET_BOOTP_DNS:
- {
- int i;
- for (i = 0; i < taglength / 4; i++)
- {
- struct grub_net_network_level_address s;
- s.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
- s.ipv4 = grub_get_unaligned32 (ptr);
- s.option = DNS_OPTION_PREFER_IPV4;
- grub_net_add_dns_server (&s);
- ptr += 4;
- }
- }
- continue;
- case GRUB_NET_BOOTP_HOSTNAME:
- grub_env_set_net_property (name, "hostname", (const char *) ptr,
- taglength);
- break;
-
- case GRUB_NET_BOOTP_DOMAIN:
- grub_env_set_net_property (name, "domain", (const char *) ptr,
- taglength);
- break;
-
- case GRUB_NET_BOOTP_ROOT_PATH:
- grub_env_set_net_property (name, "rootpath", (const char *) ptr,
- taglength);
- break;
-
- case GRUB_NET_BOOTP_EXTENSIONS_PATH:
- grub_env_set_net_property (name, "extensionspath", (const char *) ptr,
- taglength);
- break;
-
- /* If you need any other options please contact GRUB
- development team. */
+ /* FIXME RFC 3396 options concatentation */
+ if (tagtype == opt_code)
+ {
+ if (opt_len)
+ *opt_len = taglength;
+ return &ptr[i];
}
- ptr += taglength;
+ i += taglength;
}
+
+ return NULL;
}
#define OFFSET_OF(x, y) ((grub_size_t)((grub_uint8_t *)((y)->x) - (grub_uint8_t *)(y)))
@@ -143,6 +105,8 @@ grub_net_configure_by_dhcp_ack (const char *name,
struct grub_net_network_level_interface *inter;
int mask = -1;
char server_ip[sizeof ("xxx.xxx.xxx.xxx")];
+ const grub_uint8_t *opt;
+ grub_uint8_t opt_len;
addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
addr.ipv4 = bp->your_ip;
@@ -225,9 +189,69 @@ grub_net_configure_by_dhcp_ack (const char *name,
**path = 0;
}
}
- if (size > OFFSET_OF (vendor, bp))
- parse_dhcp_vendor (name, &bp->vendor, size - OFFSET_OF (vendor, bp), &mask);
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_NETMASK, &opt_len);
+ if (opt && opt_len == 4)
+ {
+ int i;
+ for (i = 0; i < 32; i++)
+ if (!(opt[i / 8] & (1 << (7 - (i % 8)))))
+ break;
+ mask = i;
+ }
grub_net_add_ipv4_local (inter, mask);
+
+ /* We do not implement dead gateway detection and the first entry SHOULD
+ be preferred one */
+ opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_ROUTER, &opt_len);
+ if (opt && opt_len && !(opt_len & 3))
+ {
+ grub_net_network_level_netaddress_t target;
+ grub_net_network_level_address_t gw;
+ char *rname;
+
+ target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
+ target.ipv4.base = 0;
+ target.ipv4.masksize = 0;
+ gw.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
+ gw.ipv4 = grub_get_unaligned32 (opt);
+ rname = grub_xasprintf ("%s:default", name);
+ if (rname)
+ grub_net_add_route_gw (rname, target, gw, 0);
+ grub_free (rname);
+ }
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_DNS, &opt_len);
+ if (opt && opt_len && !(opt_len & 3))
+ {
+ int i;
+ for (i = 0; i < opt_len / 4; i++)
+ {
+ struct grub_net_network_level_address s;
+
+ s.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
+ s.ipv4 = grub_get_unaligned32 (opt);
+ s.option = DNS_OPTION_PREFER_IPV4;
+ grub_net_add_dns_server (&s);
+ opt += 4;
+ }
+ }
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_HOSTNAME, &opt_len);
+ if (opt && opt_len)
+ grub_env_set_net_property (name, "hostname", (const char *) opt, opt_len);
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_DOMAIN, &opt_len);
+ if (opt && opt_len)
+ grub_env_set_net_property (name, "domain", (const char *) opt, opt_len);
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_ROOT_PATH, &opt_len);
+ if (opt && opt_len)
+ grub_env_set_net_property (name, "rootpath", (const char *) opt, opt_len);
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_BOOTP_EXTENSIONS_PATH, &opt_len);
+ if (opt && opt_len)
+ grub_env_set_net_property (name, "extensionspath", (const char *) opt, opt_len);
inter->dhcp_ack = grub_malloc (size);
if (inter->dhcp_ack)
@@ -286,8 +310,8 @@ grub_cmd_dhcpopt (struct grub_command *cmd __attribute__ ((unused)),
int argc, char **args)
{
struct grub_net_network_level_interface *inter;
- int num;
- grub_uint8_t *ptr;
+ unsigned num;
+ const grub_uint8_t *ptr;
grub_uint8_t taglength;
if (argc < 4)
@@ -305,44 +329,27 @@ grub_cmd_dhcpopt (struct grub_command *cmd __attribute__ ((unused)),
if (!inter->dhcp_ack)
return grub_error (GRUB_ERR_IO, N_("no DHCP info found"));
- if (inter->dhcp_acklen <= OFFSET_OF (vendor, inter->dhcp_ack))
- return grub_error (GRUB_ERR_IO, N_("no DHCP options found"));
-
- num = grub_strtoul (args[2], 0, 0);
- if (grub_errno)
- return grub_errno;
-
ptr = inter->dhcp_ack->vendor;
- if (ptr[0] != GRUB_NET_BOOTP_RFC1048_MAGIC_0
+ /* This duplicates check in find_dhcp_option to preserve previous error return */
+ if (inter->dhcp_acklen < OFFSET_OF (vendor, inter->dhcp_ack) + sizeof (grub_uint32_t)
+ || ptr[0] != GRUB_NET_BOOTP_RFC1048_MAGIC_0
|| ptr[1] != GRUB_NET_BOOTP_RFC1048_MAGIC_1
|| ptr[2] != GRUB_NET_BOOTP_RFC1048_MAGIC_2
|| ptr[3] != GRUB_NET_BOOTP_RFC1048_MAGIC_3)
return grub_error (GRUB_ERR_IO, N_("no DHCP options found"));
- ptr = ptr + sizeof (grub_uint32_t);
- while (1)
- {
- grub_uint8_t tagtype;
-
- if (ptr >= ((grub_uint8_t *) inter->dhcp_ack) + inter->dhcp_acklen)
- return grub_error (GRUB_ERR_IO, N_("no DHCP option %d found"), num);
- tagtype = *ptr++;
-
- /* Pad tag. */
- if (tagtype == 0)
- continue;
+ num = grub_strtoul (args[2], 0, 0);
+ if (grub_errno)
+ return grub_errno;
- /* End tag. */
- if (tagtype == 0xff)
- return grub_error (GRUB_ERR_IO, N_("no DHCP option %d found"), num);
+ /* Exclude PAD (0) and END (255) option codes */
+ if (num == 0 || num > 254)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid DHCP option code"));
- taglength = *ptr++;
-
- if (tagtype == num)
- break;
- ptr += taglength;
- }
+ ptr = find_dhcp_option (inter->dhcp_ack, inter->dhcp_acklen, num, &taglength);
+ if (!ptr)
+ return grub_error (GRUB_ERR_IO, N_("no DHCP option %u found"), num);
if (grub_strcmp (args[3], "string") == 0)
{
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 03/10] net: dhcp: Allow overloading legacy bootfile and name field
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
2019-03-07 15:14 ` [PATCH v3 01/10] net: dhcp: remove dead code Andre Przywara
2019-03-07 15:14 ` [PATCH v3 02/10] net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option() Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-08 11:34 ` Daniel Kiper
2019-03-07 15:14 ` [PATCH v3 04/10] net: dhcp: refactor DHCP packet transmission into separate function Andre Przywara
` (7 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
DHCP specifies a special dummy option OVERLOAD, to allow DHCP options to
spill over into the (legacy) BOOTFILE and SNAME fields.
Parse and handle this option properly.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
grub-core/net/bootp.c | 59 ++++++++++++++++++++++++++++++++++++++++++-
include/grub/net.h | 1 +
2 files changed, 59 insertions(+), 1 deletion(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 8d6763689..860d9c80d 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -25,11 +25,19 @@
#include <grub/net/udp.h>
#include <grub/datetime.h>
+enum
+{
+ GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
+ GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
+};
+
static const void *
find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
grub_uint8_t opt_code, grub_uint8_t *opt_len)
{
const grub_uint8_t *ptr;
+ grub_uint8_t overload = 0;
+ int end = 0;
grub_size_t i;
if (opt_len)
@@ -54,6 +62,7 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
size -= sizeof (*bp);
i = sizeof (grub_uint32_t);
+again:
while (i < size)
{
grub_uint8_t tagtype;
@@ -67,7 +76,10 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
/* End tag. */
if (tagtype == GRUB_NET_BOOTP_END)
- break;
+ {
+ end = 1;
+ break;
+ }
if (i >= size)
return NULL;
@@ -84,9 +96,54 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
return &ptr[i];
}
+ if (tagtype == GRUB_NET_DHCP_OVERLOAD && taglength == 1)
+ overload = ptr[i];
+
i += taglength;
}
+ if (!end)
+ return NULL;
+
+ /* RFC2131, 4.1, 23ff:
+ * If the options in a DHCP message extend into the 'sname' and 'file'
+ * fields, the 'option overload' option MUST appear in the 'options'
+ * field, with value 1, 2 or 3, as specified in RFC 1533. If the
+ * 'option overload' option is present in the 'options' field, the
+ * options in the 'options' field MUST be terminated by an 'end' option,
+ * and MAY contain one or more 'pad' options to fill the options field.
+ * The options in the 'sname' and 'file' fields (if in use as indicated
+ * by the 'options overload' option) MUST begin with the first octet of
+ * the field, MUST be terminated by an 'end' option, and MUST be
+ * followed by 'pad' options to fill the remainder of the field. Any
+ * individual option in the 'options', 'sname' and 'file' fields MUST be
+ * entirely contained in that field. The options in the 'options' field
+ * MUST be interpreted first, so that any 'option overload' options may
+ * be interpreted. The 'file' field MUST be interpreted next (if the
+ * 'option overload' option indicates that the 'file' field contains
+ * DHCP options), followed by the 'sname' field.
+ *
+ * FIXME: We do not explicitly check for trailing 'pad' options here.
+ */
+ end = 0;
+ if (overload & GRUB_DHCP_OPT_OVERLOAD_FILE)
+ {
+ overload &= ~GRUB_DHCP_OPT_OVERLOAD_FILE;
+ ptr = (grub_uint8_t *) &bp->boot_file[0];
+ size = sizeof (bp->boot_file);
+ i = 0;
+ goto again;
+ }
+
+ if (overload & GRUB_DHCP_OPT_OVERLOAD_SNAME)
+ {
+ overload &= ~GRUB_DHCP_OPT_OVERLOAD_SNAME;
+ ptr = (grub_uint8_t *) &bp->server_name[0];
+ size = sizeof (bp->server_name);
+ i = 0;
+ goto again;
+ }
+
return NULL;
}
diff --git a/include/grub/net.h b/include/grub/net.h
index 1096b2432..0c7286bd2 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -457,6 +457,7 @@ enum
GRUB_NET_BOOTP_DOMAIN = 0x0f,
GRUB_NET_BOOTP_ROOT_PATH = 0x11,
GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
+ GRUB_NET_DHCP_OVERLOAD = 52,
GRUB_NET_BOOTP_END = 0xff
};
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 04/10] net: dhcp: refactor DHCP packet transmission into separate function
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
` (2 preceding siblings ...)
2019-03-07 15:14 ` [PATCH v3 03/10] net: dhcp: Allow overloading legacy bootfile and name field Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-07 15:14 ` [PATCH v3 05/10] net: dhcp: make grub_net_process_dhcp take an interface Andre Przywara
` (6 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
In contrast to BOOTP, DHCP uses a 4-way handshake, so requires to send
packets more often.
Refactor the generation and sending of the BOOTREQUEST packet into a
separate function, so that future code can more easily reuse this.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/net/bootp.c | 146 +++++++++++++++++++++++-------------------
1 file changed, 81 insertions(+), 65 deletions(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 860d9c80d..1a7cd672b 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -31,6 +31,8 @@ enum
GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
};
+#define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
+
static const void *
find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
grub_uint8_t opt_code, grub_uint8_t *opt_len)
@@ -322,6 +324,77 @@ grub_net_configure_by_dhcp_ack (const char *name,
return inter;
}
+static grub_err_t
+send_dhcp_packet (struct grub_net_network_level_interface *iface)
+{
+ grub_err_t err;
+ struct grub_net_bootp_packet *pack;
+ struct grub_datetime date;
+ grub_int32_t t = 0;
+ struct grub_net_buff *nb;
+ struct udphdr *udph;
+ grub_net_network_level_address_t target;
+ grub_net_link_level_address_t ll_target;
+
+ nb = grub_netbuff_alloc (sizeof (*pack) + GRUB_BOOTP_MAX_OPTIONS_SIZE + 128);
+ if (!nb)
+ return grub_errno;
+
+ err = grub_netbuff_reserve (nb, sizeof (*pack) + GRUB_BOOTP_MAX_OPTIONS_SIZE + 128);
+ if (err)
+ goto out;
+
+ err = grub_netbuff_push (nb, GRUB_BOOTP_MAX_OPTIONS_SIZE);
+ if (err)
+ goto out;
+
+ grub_memset (nb->data, 0, GRUB_BOOTP_MAX_OPTIONS_SIZE);
+
+ err = grub_netbuff_push (nb, sizeof (*pack));
+ if (err)
+ goto out;
+
+ pack = (void *) nb->data;
+ grub_memset (pack, 0, sizeof (*pack));
+ pack->opcode = 1;
+ pack->hw_type = 1;
+ pack->hw_len = 6;
+ err = grub_get_datetime (&date);
+ if (err || !grub_datetime2unixtime (&date, &t))
+ {
+ grub_errno = GRUB_ERR_NONE;
+ t = 0;
+ }
+ pack->seconds = grub_cpu_to_be16 (t);
+ pack->ident = grub_cpu_to_be32 (t);
+
+ grub_memcpy (&pack->mac_addr, &iface->hwaddress.mac, 6);
+
+ grub_netbuff_push (nb, sizeof (*udph));
+
+ udph = (struct udphdr *) nb->data;
+ udph->src = grub_cpu_to_be16_compile_time (68);
+ udph->dst = grub_cpu_to_be16_compile_time (67);
+ udph->chksum = 0;
+ udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
+ target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
+ target.ipv4 = 0xffffffff;
+ err = grub_net_link_layer_resolve (iface, &target, &ll_target);
+ if (err)
+ goto out;
+
+ udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
+ &iface->address,
+ &target);
+
+ err = grub_net_send_ip_packet (iface, &target, &ll_target, nb,
+ GRUB_NET_IP_UDP);
+
+out:
+ grub_netbuff_free (nb);
+ return err;
+}
+
void
grub_net_process_dhcp (struct grub_net_buff *nb,
struct grub_net_card *card)
@@ -478,6 +551,7 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
unsigned j = 0;
int interval;
grub_err_t err;
+ unsigned i;
FOR_NET_CARDS (card)
{
@@ -506,7 +580,6 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
card->num_ifaces++;
if (!ifaces[j].name)
{
- unsigned i;
for (i = 0; i < j; i++)
grub_free (ifaces[i].name);
grub_free (ifaces);
@@ -524,78 +597,21 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
ifaces[0].prev = &grub_net_network_level_interfaces;
for (interval = 200; interval < 10000; interval *= 2)
{
- int done = 0;
+ int need_poll = 0;
for (j = 0; j < ncards; j++)
{
- struct grub_net_bootp_packet *pack;
- struct grub_datetime date;
- grub_int32_t t = 0;
- struct grub_net_buff *nb;
- struct udphdr *udph;
- grub_net_network_level_address_t target;
- grub_net_link_level_address_t ll_target;
-
if (!ifaces[j].prev)
continue;
- nb = grub_netbuff_alloc (sizeof (*pack) + 64 + 128);
- if (!nb)
- {
- grub_netbuff_free (nb);
- return grub_errno;
- }
- err = grub_netbuff_reserve (nb, sizeof (*pack) + 64 + 128);
- if (err)
- {
- grub_netbuff_free (nb);
- return err;
- }
- err = grub_netbuff_push (nb, sizeof (*pack) + 64);
+
+ err = send_dhcp_packet (&ifaces[j]);
if (err)
{
- grub_netbuff_free (nb);
- return err;
+ grub_print_error ();
+ continue;
}
- pack = (void *) nb->data;
- done = 1;
- grub_memset (pack, 0, sizeof (*pack) + 64);
- pack->opcode = 1;
- pack->hw_type = 1;
- pack->hw_len = 6;
- err = grub_get_datetime (&date);
- if (err || !grub_datetime2unixtime (&date, &t))
- {
- grub_errno = GRUB_ERR_NONE;
- t = 0;
- }
- pack->ident = grub_cpu_to_be32 (t);
- pack->seconds = grub_cpu_to_be16 (t);
-
- grub_memcpy (&pack->mac_addr, &ifaces[j].hwaddress.mac, 6);
-
- grub_netbuff_push (nb, sizeof (*udph));
-
- udph = (struct udphdr *) nb->data;
- udph->src = grub_cpu_to_be16_compile_time (68);
- udph->dst = grub_cpu_to_be16_compile_time (67);
- udph->chksum = 0;
- udph->len = grub_cpu_to_be16 (nb->tail - nb->data);
- target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
- target.ipv4 = 0xffffffff;
- err = grub_net_link_layer_resolve (&ifaces[j], &target, &ll_target);
- if (err)
- return err;
-
- udph->chksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_UDP,
- &ifaces[j].address,
- &target);
-
- err = grub_net_send_ip_packet (&ifaces[j], &target, &ll_target, nb,
- GRUB_NET_IP_UDP);
- grub_netbuff_free (nb);
- if (err)
- return err;
+ need_poll = 1;
}
- if (!done)
+ if (!need_poll)
break;
grub_net_poll_cards (interval, 0);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 05/10] net: dhcp: make grub_net_process_dhcp take an interface
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
` (3 preceding siblings ...)
2019-03-07 15:14 ` [PATCH v3 04/10] net: dhcp: refactor DHCP packet transmission into separate function Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-07 15:14 ` [PATCH v3 06/10] net: dhcp: introduce per-interface timeout Andre Przywara
` (5 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
Change the interface of the function dealing with incoming BOOTP packets
to take an interface instead of a card, to allow more fine per-interface
state (timeout, handshake state) later on.
Use the opportunity to clean up the code a bit.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/net/bootp.c | 29 ++++++++++++++---------------
grub-core/net/ip.c | 2 +-
include/grub/net.h | 2 +-
3 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 1a7cd672b..5bb5b3d27 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -395,12 +395,19 @@ out:
return err;
}
+/*
+ * This is called directly from net/ip.c:handle_dgram(), because those
+ * BOOTP/DHCP packets are a bit special due to their improper
+ * sender/receiver IP fields.
+ */
void
grub_net_process_dhcp (struct grub_net_buff *nb,
- struct grub_net_card *card)
+ struct grub_net_network_level_interface *iface)
{
char *name;
- struct grub_net_network_level_interface *inf;
+ struct grub_net_card *card = iface->card;
+ const struct grub_net_bootp_packet *bp = (const struct grub_net_bootp_packet *) nb->data;
+ grub_size_t size = nb->tail - nb->data;
name = grub_xasprintf ("%s:dhcp", card->name);
if (!name)
@@ -408,23 +415,15 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
grub_print_error ();
return;
}
- grub_net_configure_by_dhcp_ack (name, card,
- 0, (const struct grub_net_bootp_packet *) nb->data,
- (nb->tail - nb->data), 0, 0, 0);
+ grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
grub_free (name);
if (grub_errno)
grub_print_error ();
else
- {
- FOR_NET_NETWORK_LEVEL_INTERFACES(inf)
- if (grub_memcmp (inf->name, card->name, grub_strlen (card->name)) == 0
- && grub_memcmp (inf->name + grub_strlen (card->name),
- ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
- {
- grub_net_network_level_interface_unregister (inf);
- break;
- }
- }
+ if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 0 &&
+ grub_memcmp (iface->name + grub_strlen (card->name),
+ ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
+ grub_net_network_level_interface_unregister (iface);
}
static char
diff --git a/grub-core/net/ip.c b/grub-core/net/ip.c
index 7c95cc746..ea5edf8f1 100644
--- a/grub-core/net/ip.c
+++ b/grub-core/net/ip.c
@@ -279,7 +279,7 @@ handle_dgram (struct grub_net_buff *nb,
&& grub_memcmp (inf->hwaddress.mac, &bootp->mac_addr,
sizeof (inf->hwaddress.mac)) == 0)
{
- grub_net_process_dhcp (nb, inf->card);
+ grub_net_process_dhcp (nb, inf);
grub_netbuff_free (nb);
return GRUB_ERR_NONE;
}
diff --git a/include/grub/net.h b/include/grub/net.h
index 0c7286bd2..3f649d753 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -475,7 +475,7 @@ grub_net_add_ipv4_local (struct grub_net_network_level_interface *inf,
void
grub_net_process_dhcp (struct grub_net_buff *nb,
- struct grub_net_card *card);
+ struct grub_net_network_level_interface *iface);
int
grub_net_hwaddr_cmp (const grub_net_link_level_address_t *a,
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 06/10] net: dhcp: introduce per-interface timeout
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
` (4 preceding siblings ...)
2019-03-07 15:14 ` [PATCH v3 05/10] net: dhcp: make grub_net_process_dhcp take an interface Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-07 15:14 ` [PATCH v3 07/10] net: dhcp: use DHCP options for name and bootfile Andre Przywara
` (4 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
Currently we have a global timeout for all network cards in the
BOOTP/DHCP discovery process.
Make this timeout a per-interface one, so better accommodate the
upcoming 4-way DHCP handshake and to also cover the lease time limit a
DHCP offer will come with.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/net/bootp.c | 38 ++++++++++++++++++++++++++++++++++----
include/grub/net.h | 2 ++
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 5bb5b3d27..2fb79d162 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -33,6 +33,9 @@ enum
#define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
+/* Max timeout when waiting for BOOTP/DHCP reply */
+#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
+
static const void *
find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
grub_uint8_t opt_code, grub_uint8_t *opt_len)
@@ -548,7 +551,6 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
struct grub_net_network_level_interface *ifaces;
grub_size_t ncards = 0;
unsigned j = 0;
- int interval;
grub_err_t err;
unsigned i;
@@ -587,6 +589,7 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
ifaces[j].address.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_DHCP_RECV;
grub_memcpy (&ifaces[j].hwaddress, &card->default_address,
sizeof (ifaces[j].hwaddress));
+ ifaces[j].dhcp_tmo = ifaces[j].dhcp_tmo_left = 1;
j++;
}
ifaces[ncards - 1].next = grub_net_network_level_interfaces;
@@ -594,25 +597,52 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
grub_net_network_level_interfaces->prev = & ifaces[ncards - 1].next;
grub_net_network_level_interfaces = &ifaces[0];
ifaces[0].prev = &grub_net_network_level_interfaces;
- for (interval = 200; interval < 10000; interval *= 2)
+
+ /*
+ * Running DHCP restransmission timer is kept per interface in dhcp_tmo_left.
+ * When it runs off, dhcp_tmo is increased exponentionally and dhcp_tmo_left
+ * initialized to it. Max value is 32 which gives approximately 12s total per
+ * packet timeout assuming 200ms poll tick. Timeout is reset when DHCP OFFER
+ * is received, so total timeout is 25s in the worst case.
+ *
+ * DHCP NAK also resets timer and transaction starts again.
+ *
+ * Total wait time is limited to ~25s to prevent endless loop in case of
+ * permanent NAK
+ */
+ for (i = 0; i < GRUB_DHCP_MAX_PACKET_TIMEOUT * 4; i++)
{
int need_poll = 0;
for (j = 0; j < ncards; j++)
{
- if (!ifaces[j].prev)
+ if (!ifaces[j].prev ||
+ ifaces[j].dhcp_tmo > GRUB_DHCP_MAX_PACKET_TIMEOUT)
+ continue;
+
+ if (--ifaces[j].dhcp_tmo_left)
+ {
+ need_poll = 1;
+ continue;
+ }
+
+ ifaces[j].dhcp_tmo *= 2;
+ if (ifaces[j].dhcp_tmo > GRUB_DHCP_MAX_PACKET_TIMEOUT)
continue;
err = send_dhcp_packet (&ifaces[j]);
if (err)
{
grub_print_error ();
+ /* To ignore it during next poll */
+ ifaces[j].dhcp_tmo = GRUB_DHCP_MAX_PACKET_TIMEOUT + 1;
continue;
}
+ ifaces[j].dhcp_tmo_left = ifaces[j].dhcp_tmo;
need_poll = 1;
}
if (!need_poll)
break;
- grub_net_poll_cards (interval, 0);
+ grub_net_poll_cards (200, 0);
}
err = GRUB_ERR_NONE;
diff --git a/include/grub/net.h b/include/grub/net.h
index 3f649d753..a1138f5d4 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -292,6 +292,8 @@ struct grub_net_network_level_interface
struct grub_net_bootp_packet *dhcp_ack;
grub_size_t dhcp_acklen;
grub_uint16_t vlantag;
+ unsigned dhcp_tmo_left; /* DHCPv4 running retransmission timeout */
+ unsigned dhcp_tmo; /* DHCPv4 current retransmission timeout */
void *data;
};
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 07/10] net: dhcp: use DHCP options for name and bootfile
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
` (5 preceding siblings ...)
2019-03-07 15:14 ` [PATCH v3 06/10] net: dhcp: introduce per-interface timeout Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-08 11:48 ` Daniel Kiper
2019-03-07 15:14 ` [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets Andre Przywara
` (3 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
The BOOTP RFC describes the boot file name and the server name as being
part of the integral BOOTP data structure, with some limits on the size
of them.
DHCP extends this by allowing them to be separate DHCP options, which is
more flexible.
Teach the code dealing with those fields to check for those DHCP options
first and use this information, if provided. We fall back to using the
BOOTP information if those options are not used.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
grub-core/net/bootp.c | 72 +++++++++++++++++++++++++++++++------------
include/grub/net.h | 2 ++
2 files changed, 54 insertions(+), 20 deletions(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 2fb79d162..bfde6ef10 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -168,7 +168,9 @@ grub_net_configure_by_dhcp_ack (const char *name,
int mask = -1;
char server_ip[sizeof ("xxx.xxx.xxx.xxx")];
const grub_uint8_t *opt;
- grub_uint8_t opt_len;
+ grub_uint8_t opt_len, overload = 0;
+ const char *boot_file = 0, *server_name = 0;
+ grub_size_t boot_file_len, server_name_len;
addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
addr.ipv4 = bp->your_ip;
@@ -187,9 +189,36 @@ grub_net_configure_by_dhcp_ack (const char *name,
if (!inter)
return 0;
- if (size > OFFSET_OF (boot_file, bp))
- grub_env_set_net_property (name, "boot_file", bp->boot_file,
- sizeof (bp->boot_file));
+ opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_OVERLOAD, &opt_len);
+ if (opt && opt_len == 1)
+ overload = *opt;
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_TFTP_SERVER_NAME, &opt_len);
+ if (opt && opt_len)
+ {
+ server_name = (const char *) opt;
+ server_name_len = opt_len;
+ }
+ else if (size > OFFSET_OF (server_name, bp) && !(overload & GRUB_DHCP_OPT_OVERLOAD_SNAME) &&
+ bp->server_name[0])
+ {
+ server_name = bp->server_name;
+ server_name_len = sizeof (bp->server_name);
+ }
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_BOOTFILE_NAME, &opt_len);
+ if (opt && opt_len)
+ {
+ boot_file = (const char *) opt;
+ boot_file_len = opt_len;
+ }
+ else if (size > OFFSET_OF (boot_file, bp) && !(overload && GRUB_DHCP_OPT_OVERLOAD_FILE) &&
+ bp->boot_file[0])
+ {
+ boot_file = bp->boot_file;
+ boot_file_len = sizeof (bp->boot_file);
+ }
+
if (bp->server_ip)
{
grub_snprintf (server_ip, sizeof (server_ip), "%d.%d.%d.%d",
@@ -220,35 +249,38 @@ grub_net_configure_by_dhcp_ack (const char *name,
*device = grub_xasprintf ("tftp,%s", server_ip);
grub_print_error ();
}
- if (size > OFFSET_OF (server_name, bp)
- && bp->server_name[0])
+
+ if (server_name)
{
- grub_env_set_net_property (name, "dhcp_server_name", bp->server_name,
- sizeof (bp->server_name));
+ grub_env_set_net_property (name, "dhcp_server_name", server_name, server_name_len);
if (is_def && !grub_net_default_server)
{
- grub_net_default_server = grub_strdup (bp->server_name);
+ grub_net_default_server = grub_strdup (server_name);
grub_print_error ();
}
if (device && !*device)
{
- *device = grub_xasprintf ("tftp,%s", bp->server_name);
+ *device = grub_xasprintf ("tftp,%s", server_name);
grub_print_error ();
}
}
- if (size > OFFSET_OF (boot_file, bp) && path)
+ if (boot_file)
{
- *path = grub_strndup (bp->boot_file, sizeof (bp->boot_file));
- grub_print_error ();
- if (*path)
+ grub_env_set_net_property (name, "boot_file", boot_file, boot_file_len);
+ if (path)
{
- char *slash;
- slash = grub_strrchr (*path, '/');
- if (slash)
- *slash = 0;
- else
- **path = 0;
+ *path = grub_strndup (boot_file, boot_file_len);
+ grub_print_error ();
+ if (*path)
+ {
+ char *slash;
+ slash = grub_strrchr (*path, '/');
+ if (slash)
+ *slash = 0;
+ else
+ **path = 0;
+ }
}
}
diff --git a/include/grub/net.h b/include/grub/net.h
index a1138f5d4..f7b546446 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -460,6 +460,8 @@ enum
GRUB_NET_BOOTP_ROOT_PATH = 0x11,
GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
GRUB_NET_DHCP_OVERLOAD = 52,
+ GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
+ GRUB_NET_DHCP_BOOTFILE_NAME = 67,
GRUB_NET_BOOTP_END = 0xff
};
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
` (6 preceding siblings ...)
2019-03-07 15:14 ` [PATCH v3 07/10] net: dhcp: use DHCP options for name and bootfile Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-08 12:01 ` Daniel Kiper
2019-03-07 15:14 ` [PATCH v3 09/10] net: dhcp: actually send out DHCPv4 DISCOVER and REQUEST messages Andre Przywara
` (2 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
In respone to a BOOTREQUEST packet a BOOTP server would answer with a
BOOTREPLY packet, which ends the conversation for good.
DHCP uses a 4-way handshake, where the initial server respone is an OFFER,
which has to be answered with REQUEST by the client again, only to be
completed by an ACKNOWLEDGE packet from the server.
Teach the grub_net_process_dhcp() function to deal with OFFER packets,
and treat ACK packets the same es BOOTREPLY packets.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/net/bootp.c | 78 +++++++++++++++++++++++++++++++++++--------
include/grub/net.h | 5 +++
2 files changed, 70 insertions(+), 13 deletions(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index bfde6ef10..d0482a7d5 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -30,6 +30,21 @@ enum
GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
};
+enum
+{
+ GRUB_DHCP_MESSAGE_UNKNOWN,
+ GRUB_DHCP_MESSAGE_DISCOVER,
+ GRUB_DHCP_MESSAGE_OFFER,
+ GRUB_DHCP_MESSAGE_REQUEST,
+ GRUB_DHCP_MESSAGE_DECLINE,
+ GRUB_DHCP_MESSAGE_ACK,
+ GRUB_DHCP_MESSAGE_NAK,
+ GRUB_DHCP_MESSAGE_RELEASE,
+ GRUB_DHCP_MESSAGE_INFORM,
+};
+
+/* Max timeout when waiting for BOOTP/DHCP reply */
+#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
#define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
@@ -443,22 +458,59 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
struct grub_net_card *card = iface->card;
const struct grub_net_bootp_packet *bp = (const struct grub_net_bootp_packet *) nb->data;
grub_size_t size = nb->tail - nb->data;
+ const grub_uint8_t *opt;
+ grub_uint8_t opt_len, type;
+ grub_uint32_t srv_id = 0;
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_MESSAGE_TYPE, &opt_len);
+ if (opt && opt_len == 1)
+ type = *opt;
+ else
+ type = GRUB_DHCP_MESSAGE_UNKNOWN;
+
+ opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_SERVER_IDENTIFIER, &opt_len);
+ if (opt && opt_len == sizeof (srv_id))
+ srv_id = grub_get_unaligned32 (opt);
- name = grub_xasprintf ("%s:dhcp", card->name);
- if (!name)
+ /*
+ * If we received BOOTP reply or DHCPACK, proceed with configuration.
+ * Otherwise store offered address and server id for later processing
+ * of DHCPACK.
+ * xid and srv_id are stored in network order so do not need conversion.
+ */
+ if ((!iface->srv_id && type == GRUB_DHCP_MESSAGE_UNKNOWN)
+ || (iface->srv_id && type == GRUB_DHCP_MESSAGE_ACK
+ && bp->ident == iface->xid
+ && srv_id == iface->srv_id))
{
- grub_print_error ();
- return;
+ name = grub_xasprintf ("%s:dhcp", card->name);
+ if (!name)
+ {
+ grub_print_error ();
+ return;
+ }
+ grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
+ grub_free (name);
+ if (grub_errno)
+ grub_print_error ();
+ else
+ grub_net_network_level_interface_unregister (iface);
+ }
+ else if (!iface->srv_id && type == GRUB_DHCP_MESSAGE_OFFER && srv_id)
+ {
+ iface->srv_id = srv_id;
+ iface->my_ip = bp->your_ip;
+ /* Reset retransmission timer */
+ iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
+ }
+ else if (iface->srv_id && type == GRUB_DHCP_MESSAGE_NAK
+ && bp->ident == iface->xid
+ && srv_id == iface->srv_id)
+ {
+ iface->xid = iface->srv_id = iface->my_ip = 0;
+ /* Reset retransmission timer */
+ iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
}
- grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
- grub_free (name);
- if (grub_errno)
- grub_print_error ();
- else
- if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 0 &&
- grub_memcmp (iface->name + grub_strlen (card->name),
- ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
- grub_net_network_level_interface_unregister (iface);
}
static char
diff --git a/include/grub/net.h b/include/grub/net.h
index f7b546446..68a9f1311 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -292,6 +292,9 @@ struct grub_net_network_level_interface
struct grub_net_bootp_packet *dhcp_ack;
grub_size_t dhcp_acklen;
grub_uint16_t vlantag;
+ grub_uint32_t xid; /* DHCPv4 transaction id */
+ grub_uint32_t srv_id; /* DHCPv4 server_identifier */
+ grub_uint32_t my_ip; /* DHCPv4 offered IP address */
unsigned dhcp_tmo_left; /* DHCPv4 running retransmission timeout */
unsigned dhcp_tmo; /* DHCPv4 current retransmission timeout */
void *data;
@@ -460,6 +463,8 @@ enum
GRUB_NET_BOOTP_ROOT_PATH = 0x11,
GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
GRUB_NET_DHCP_OVERLOAD = 52,
+ GRUB_NET_DHCP_MESSAGE_TYPE = 53,
+ GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
GRUB_NET_DHCP_BOOTFILE_NAME = 67,
GRUB_NET_BOOTP_END = 0xff
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 09/10] net: dhcp: actually send out DHCPv4 DISCOVER and REQUEST messages
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
` (7 preceding siblings ...)
2019-03-07 15:14 ` [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-07 15:14 ` [PATCH v3 10/10] net: dhcp: add explicit net_dhcp command Andre Przywara
2019-03-08 16:03 ` [PATCH v3 00/10] net: bootp: add native DHCPv4 support Daniel Kiper
10 siblings, 0 replies; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
Even though we were parsing some DHCP options sent by the server, so far
we are only using the BOOTP 2-way handshake, even when talking to a DHCP
server.
Change this by actually sending out DHCP DISCOVER packets instead of the
generic (mostly empty) BOOTP BOOTREQUEST packets.
A pure BOOTP server would ignore the extra DHCP options in the DISCOVER
packet and would just reply with a BOOTREPLY packet, which we also
handle in the code.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/net/bootp.c | 120 +++++++++++++++++++++++++++++++++++++++++-
include/grub/net.h | 2 +
2 files changed, 121 insertions(+), 1 deletion(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index d0482a7d5..63763aaab 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -25,6 +25,48 @@
#include <grub/net/udp.h>
#include <grub/datetime.h>
+struct grub_dhcp_discover_options
+{
+ grub_uint8_t magic[4];
+ struct
+ {
+ grub_uint8_t code;
+ grub_uint8_t len;
+ grub_uint8_t data;
+ } GRUB_PACKED message_type;
+ grub_uint8_t end;
+} GRUB_PACKED;
+
+struct grub_dhcp_request_options
+{
+ grub_uint8_t magic[4];
+ struct
+ {
+ grub_uint8_t code;
+ grub_uint8_t len;
+ grub_uint8_t data;
+ } GRUB_PACKED message_type;
+ struct
+ {
+ grub_uint8_t type;
+ grub_uint8_t len;
+ grub_uint32_t data;
+ } GRUB_PACKED server_identifier;
+ struct
+ {
+ grub_uint8_t type;
+ grub_uint8_t len;
+ grub_uint32_t data;
+ } GRUB_PACKED requested_ip;
+ struct
+ {
+ grub_uint8_t type;
+ grub_uint8_t len;
+ grub_uint8_t data[7];
+ } GRUB_PACKED parameter_request;
+ grub_uint8_t end;
+} GRUB_PACKED;
+
enum
{
GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
@@ -43,6 +85,8 @@ enum
GRUB_DHCP_MESSAGE_INFORM,
};
+#define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
+
/* Max timeout when waiting for BOOTP/DHCP reply */
#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
@@ -386,6 +430,64 @@ send_dhcp_packet (struct grub_net_network_level_interface *iface)
grub_net_network_level_address_t target;
grub_net_link_level_address_t ll_target;
+ static struct grub_dhcp_discover_options discover_options =
+ {
+ {
+ GRUB_NET_BOOTP_RFC1048_MAGIC_0,
+ GRUB_NET_BOOTP_RFC1048_MAGIC_1,
+ GRUB_NET_BOOTP_RFC1048_MAGIC_2,
+ GRUB_NET_BOOTP_RFC1048_MAGIC_3,
+ },
+ {
+ GRUB_NET_DHCP_MESSAGE_TYPE,
+ sizeof (discover_options.message_type.data),
+ GRUB_DHCP_MESSAGE_DISCOVER,
+ },
+ GRUB_NET_BOOTP_END,
+ };
+
+ static struct grub_dhcp_request_options request_options =
+ {
+ {
+ GRUB_NET_BOOTP_RFC1048_MAGIC_0,
+ GRUB_NET_BOOTP_RFC1048_MAGIC_1,
+ GRUB_NET_BOOTP_RFC1048_MAGIC_2,
+ GRUB_NET_BOOTP_RFC1048_MAGIC_3,
+ },
+ {
+ GRUB_NET_DHCP_MESSAGE_TYPE,
+ sizeof (request_options.message_type.data),
+ GRUB_DHCP_MESSAGE_REQUEST,
+ },
+ {
+ GRUB_NET_DHCP_SERVER_IDENTIFIER,
+ sizeof (request_options.server_identifier.data),
+ 0,
+ },
+ {
+ GRUB_NET_DHCP_REQUESTED_IP_ADDRESS,
+ sizeof (request_options.requested_ip.data),
+ 0,
+ },
+ {
+ GRUB_NET_DHCP_PARAMETER_REQUEST_LIST,
+ sizeof (request_options.parameter_request.data),
+ {
+ GRUB_NET_BOOTP_NETMASK,
+ GRUB_NET_BOOTP_ROUTER,
+ GRUB_NET_BOOTP_DNS,
+ GRUB_NET_BOOTP_DOMAIN,
+ GRUB_NET_BOOTP_HOSTNAME,
+ GRUB_NET_BOOTP_ROOT_PATH,
+ GRUB_NET_BOOTP_EXTENSIONS_PATH,
+ },
+ },
+ GRUB_NET_BOOTP_END,
+ };
+
+ COMPILE_TIME_ASSERT (sizeof (discover_options) <= GRUB_BOOTP_MAX_OPTIONS_SIZE);
+ COMPILE_TIME_ASSERT (sizeof (request_options) <= GRUB_BOOTP_MAX_OPTIONS_SIZE);
+
nb = grub_netbuff_alloc (sizeof (*pack) + GRUB_BOOTP_MAX_OPTIONS_SIZE + 128);
if (!nb)
return grub_errno;
@@ -399,6 +501,19 @@ send_dhcp_packet (struct grub_net_network_level_interface *iface)
goto out;
grub_memset (nb->data, 0, GRUB_BOOTP_MAX_OPTIONS_SIZE);
+ if (!iface->srv_id)
+ {
+ grub_memcpy (nb->data, &discover_options, sizeof (discover_options));
+ }
+ else
+ {
+ struct grub_dhcp_request_options *ro = (struct grub_dhcp_request_options *) nb->data;
+
+ grub_memcpy (nb->data, &request_options, sizeof (request_options));
+ /* my_ip and srv_id are stored in network order so do not need conversion. */
+ grub_set_unaligned32 (&ro->server_identifier.data, iface->srv_id);
+ grub_set_unaligned32 (&ro->requested_ip.data, iface->my_ip);
+ }
err = grub_netbuff_push (nb, sizeof (*pack));
if (err)
@@ -416,7 +531,10 @@ send_dhcp_packet (struct grub_net_network_level_interface *iface)
t = 0;
}
pack->seconds = grub_cpu_to_be16 (t);
- pack->ident = grub_cpu_to_be32 (t);
+ if (!iface->srv_id)
+ iface->xid = pack->ident = grub_cpu_to_be32 (t);
+ else
+ pack->ident = iface->xid;
grub_memcpy (&pack->mac_addr, &iface->hwaddress.mac, 6);
diff --git a/include/grub/net.h b/include/grub/net.h
index 68a9f1311..4a9069a14 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -462,9 +462,11 @@ enum
GRUB_NET_BOOTP_DOMAIN = 0x0f,
GRUB_NET_BOOTP_ROOT_PATH = 0x11,
GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
+ GRUB_NET_DHCP_REQUESTED_IP_ADDRESS = 50,
GRUB_NET_DHCP_OVERLOAD = 52,
GRUB_NET_DHCP_MESSAGE_TYPE = 53,
GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
+ GRUB_NET_DHCP_PARAMETER_REQUEST_LIST = 55,
GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
GRUB_NET_DHCP_BOOTFILE_NAME = 67,
GRUB_NET_BOOTP_END = 0xff
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 10/10] net: dhcp: add explicit net_dhcp command
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
` (8 preceding siblings ...)
2019-03-07 15:14 ` [PATCH v3 09/10] net: dhcp: actually send out DHCPv4 DISCOVER and REQUEST messages Andre Przywara
@ 2019-03-07 15:14 ` Andre Przywara
2019-03-08 16:03 ` [PATCH v3 00/10] net: bootp: add native DHCPv4 support Daniel Kiper
10 siblings, 0 replies; 21+ messages in thread
From: Andre Przywara @ 2019-03-07 15:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir Serbinenko, Andrei Borzenkov, Daniel Kiper,
Mark Rutland, grub-devel
From: Andrei Borzenkov <arvidjaar@gmail.com>
Mostly for cosmetic reasons, we add a "net_dhcp" command, which is
(at the moment) identical to the existing "net_bootp" command. Both
actually trigger a DHCP handshake now, and both should be able to deal
with pure BOOTP servers.
We could think about dropping the DHCP options from the initial DISCOVER
packet when the user issues the net_bootp command, but it's unclear
whether this is really useful, as both protocols should be able to
coexist.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/net/bootp.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 63763aaab..04cfbb045 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -864,7 +864,7 @@ grub_cmd_bootp (struct grub_command *cmd __attribute__ ((unused)),
return err;
}
-static grub_command_t cmd_getdhcp, cmd_bootp;
+static grub_command_t cmd_getdhcp, cmd_bootp, cmd_dhcp;
void
grub_bootp_init (void)
@@ -872,6 +872,9 @@ grub_bootp_init (void)
cmd_bootp = grub_register_command ("net_bootp", grub_cmd_bootp,
N_("[CARD]"),
N_("perform a bootp autoconfiguration"));
+ cmd_dhcp = grub_register_command ("net_dhcp", grub_cmd_bootp,
+ N_("[CARD]"),
+ N_("perform a DHCP autoconfiguration"));
cmd_getdhcp = grub_register_command ("net_get_dhcp_option", grub_cmd_dhcpopt,
N_("VAR INTERFACE NUMBER DESCRIPTION"),
N_("retrieve DHCP option and save it into VAR. If VAR is - then print the value."));
@@ -882,4 +885,5 @@ grub_bootp_fini (void)
{
grub_unregister_command (cmd_getdhcp);
grub_unregister_command (cmd_bootp);
+ grub_unregister_command (cmd_dhcp);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 02/10] net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option()
2019-03-07 15:14 ` [PATCH v3 02/10] net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option() Andre Przywara
@ 2019-03-08 11:29 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2019-03-08 11:29 UTC (permalink / raw)
To: Andre Przywara
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Thu, Mar 07, 2019 at 03:14:08PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> For proper DHCP support we will need to parse DHCP options from a packet
> more often and at various places.
>
> Refactor the option parsing into a new function, which will scan a
> packet to find *a particular* option field.
> Use that new function in places where we were dealing with DHCP options
> before.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 03/10] net: dhcp: Allow overloading legacy bootfile and name field
2019-03-07 15:14 ` [PATCH v3 03/10] net: dhcp: Allow overloading legacy bootfile and name field Andre Przywara
@ 2019-03-08 11:34 ` Daniel Kiper
2019-03-08 11:51 ` Andre Przywara
0 siblings, 1 reply; 21+ messages in thread
From: Daniel Kiper @ 2019-03-08 11:34 UTC (permalink / raw)
To: Andre Przywara
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Thu, Mar 07, 2019 at 03:14:09PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> DHCP specifies a special dummy option OVERLOAD, to allow DHCP options to
> spill over into the (legacy) BOOTFILE and SNAME fields.
>
> Parse and handle this option properly.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> but one
nitpick below...
> ---
> grub-core/net/bootp.c | 59 ++++++++++++++++++++++++++++++++++++++++++-
> include/grub/net.h | 1 +
> 2 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 8d6763689..860d9c80d 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -25,11 +25,19 @@
> #include <grub/net/udp.h>
> #include <grub/datetime.h>
>
> +enum
> +{
> + GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
> + GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
> +};
> +
> static const void *
> find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> grub_uint8_t opt_code, grub_uint8_t *opt_len)
> {
> const grub_uint8_t *ptr;
> + grub_uint8_t overload = 0;
> + int end = 0;
> grub_size_t i;
>
> if (opt_len)
> @@ -54,6 +62,7 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> size -= sizeof (*bp);
> i = sizeof (grub_uint32_t);
>
> +again:
> while (i < size)
> {
> grub_uint8_t tagtype;
> @@ -67,7 +76,10 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
>
> /* End tag. */
> if (tagtype == GRUB_NET_BOOTP_END)
> - break;
> + {
> + end = 1;
> + break;
> + }
>
> if (i >= size)
> return NULL;
> @@ -84,9 +96,54 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> return &ptr[i];
> }
>
> + if (tagtype == GRUB_NET_DHCP_OVERLOAD && taglength == 1)
> + overload = ptr[i];
> +
> i += taglength;
> }
>
> + if (!end)
> + return NULL;
> +
> + /* RFC2131, 4.1, 23ff:
> + * If the options in a DHCP message extend into the 'sname' and 'file'
> + * fields, the 'option overload' option MUST appear in the 'options'
> + * field, with value 1, 2 or 3, as specified in RFC 1533. If the
> + * 'option overload' option is present in the 'options' field, the
> + * options in the 'options' field MUST be terminated by an 'end' option,
> + * and MAY contain one or more 'pad' options to fill the options field.
> + * The options in the 'sname' and 'file' fields (if in use as indicated
> + * by the 'options overload' option) MUST begin with the first octet of
> + * the field, MUST be terminated by an 'end' option, and MUST be
> + * followed by 'pad' options to fill the remainder of the field. Any
> + * individual option in the 'options', 'sname' and 'file' fields MUST be
> + * entirely contained in that field. The options in the 'options' field
> + * MUST be interpreted first, so that any 'option overload' options may
> + * be interpreted. The 'file' field MUST be interpreted next (if the
> + * 'option overload' option indicates that the 'file' field contains
> + * DHCP options), followed by the 'sname' field.
> + *
> + * FIXME: We do not explicitly check for trailing 'pad' options here.
> + */
> + end = 0;
It seems to me that this line is not needed... I can drop it before
committing the patch if you are OK with it.
> + if (overload & GRUB_DHCP_OPT_OVERLOAD_FILE)
> + {
> + overload &= ~GRUB_DHCP_OPT_OVERLOAD_FILE;
> + ptr = (grub_uint8_t *) &bp->boot_file[0];
> + size = sizeof (bp->boot_file);
> + i = 0;
> + goto again;
> + }
> +
> + if (overload & GRUB_DHCP_OPT_OVERLOAD_SNAME)
> + {
> + overload &= ~GRUB_DHCP_OPT_OVERLOAD_SNAME;
> + ptr = (grub_uint8_t *) &bp->server_name[0];
> + size = sizeof (bp->server_name);
> + i = 0;
> + goto again;
> + }
> +
> return NULL;
> }
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 07/10] net: dhcp: use DHCP options for name and bootfile
2019-03-07 15:14 ` [PATCH v3 07/10] net: dhcp: use DHCP options for name and bootfile Andre Przywara
@ 2019-03-08 11:48 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2019-03-08 11:48 UTC (permalink / raw)
To: Andre Przywara
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Thu, Mar 07, 2019 at 03:14:13PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> The BOOTP RFC describes the boot file name and the server name as being
> part of the integral BOOTP data structure, with some limits on the size
> of them.
> DHCP extends this by allowing them to be separate DHCP options, which is
> more flexible.
>
> Teach the code dealing with those fields to check for those DHCP options
> first and use this information, if provided. We fall back to using the
> BOOTP information if those options are not used.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 03/10] net: dhcp: Allow overloading legacy bootfile and name field
2019-03-08 11:34 ` Daniel Kiper
@ 2019-03-08 11:51 ` Andre Przywara
2019-03-08 12:04 ` Daniel Kiper
0 siblings, 1 reply; 21+ messages in thread
From: Andre Przywara @ 2019-03-08 11:51 UTC (permalink / raw)
To: Daniel Kiper
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Fri, 8 Mar 2019 12:34:18 +0100
Daniel Kiper <daniel.kiper@oracle.com> wrote:
> On Thu, Mar 07, 2019 at 03:14:09PM +0000, Andre Przywara wrote:
> > From: Andrei Borzenkov <arvidjaar@gmail.com>
> >
> > DHCP specifies a special dummy option OVERLOAD, to allow DHCP options to
> > spill over into the (legacy) BOOTFILE and SNAME fields.
> >
> > Parse and handle this option properly.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>
> In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> but one
> nitpick below...
>
> > ---
> > grub-core/net/bootp.c | 59 ++++++++++++++++++++++++++++++++++++++++++-
> > include/grub/net.h | 1 +
> > 2 files changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > index 8d6763689..860d9c80d 100644
> > --- a/grub-core/net/bootp.c
> > +++ b/grub-core/net/bootp.c
> > @@ -25,11 +25,19 @@
> > #include <grub/net/udp.h>
> > #include <grub/datetime.h>
> >
> > +enum
> > +{
> > + GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
> > + GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
> > +};
> > +
> > static const void *
> > find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> > grub_uint8_t opt_code, grub_uint8_t *opt_len)
> > {
> > const grub_uint8_t *ptr;
> > + grub_uint8_t overload = 0;
> > + int end = 0;
> > grub_size_t i;
> >
> > if (opt_len)
> > @@ -54,6 +62,7 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> > size -= sizeof (*bp);
> > i = sizeof (grub_uint32_t);
> >
> > +again:
> > while (i < size)
> > {
> > grub_uint8_t tagtype;
> > @@ -67,7 +76,10 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> >
> > /* End tag. */
> > if (tagtype == GRUB_NET_BOOTP_END)
> > - break;
> > + {
> > + end = 1;
> > + break;
> > + }
> >
> > if (i >= size)
> > return NULL;
> > @@ -84,9 +96,54 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> > return &ptr[i];
> > }
> >
> > + if (tagtype == GRUB_NET_DHCP_OVERLOAD && taglength == 1)
> > + overload = ptr[i];
> > +
> > i += taglength;
> > }
> >
> > + if (!end)
> > + return NULL;
> > +
> > + /* RFC2131, 4.1, 23ff:
> > + * If the options in a DHCP message extend into the 'sname' and 'file'
> > + * fields, the 'option overload' option MUST appear in the 'options'
> > + * field, with value 1, 2 or 3, as specified in RFC 1533. If the
> > + * 'option overload' option is present in the 'options' field, the
> > + * options in the 'options' field MUST be terminated by an 'end' option,
> > + * and MAY contain one or more 'pad' options to fill the options field.
> > + * The options in the 'sname' and 'file' fields (if in use as indicated
> > + * by the 'options overload' option) MUST begin with the first octet of
> > + * the field, MUST be terminated by an 'end' option, and MUST be
> > + * followed by 'pad' options to fill the remainder of the field. Any
> > + * individual option in the 'options', 'sname' and 'file' fields MUST be
> > + * entirely contained in that field. The options in the 'options' field
> > + * MUST be interpreted first, so that any 'option overload' options may
> > + * be interpreted. The 'file' field MUST be interpreted next (if the
> > + * 'option overload' option indicates that the 'file' field contains
> > + * DHCP options), followed by the 'sname' field.
> > + *
> > + * FIXME: We do not explicitly check for trailing 'pad' options here.
> > + */
> > + end = 0;
>
> It seems to me that this line is not needed... I can drop it before
> committing the patch if you are OK with it.
I was eyeballing this line as well, but I think it *is* needed:
It resets end to 0, before possibly jumping back to the again label, inside the if conditions below.
Does that make sense?
Cheers,
Andre.
>
> > + if (overload & GRUB_DHCP_OPT_OVERLOAD_FILE)
> > + {
> > + overload &= ~GRUB_DHCP_OPT_OVERLOAD_FILE;
> > + ptr = (grub_uint8_t *) &bp->boot_file[0];
> > + size = sizeof (bp->boot_file);
> > + i = 0;
> > + goto again;
> > + }
> > +
> > + if (overload & GRUB_DHCP_OPT_OVERLOAD_SNAME)
> > + {
> > + overload &= ~GRUB_DHCP_OPT_OVERLOAD_SNAME;
> > + ptr = (grub_uint8_t *) &bp->server_name[0];
> > + size = sizeof (bp->server_name);
> > + i = 0;
> > + goto again;
> > + }
> > +
> > return NULL;
> > }
>
> Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets
2019-03-07 15:14 ` [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets Andre Przywara
@ 2019-03-08 12:01 ` Daniel Kiper
2019-03-08 12:21 ` Andre Przywara
0 siblings, 1 reply; 21+ messages in thread
From: Daniel Kiper @ 2019-03-08 12:01 UTC (permalink / raw)
To: Andre Przywara
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Thu, Mar 07, 2019 at 03:14:14PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <arvidjaar@gmail.com>
>
> In respone to a BOOTREQUEST packet a BOOTP server would answer with a
> BOOTREPLY packet, which ends the conversation for good.
> DHCP uses a 4-way handshake, where the initial server respone is an OFFER,
> which has to be answered with REQUEST by the client again, only to be
> completed by an ACKNOWLEDGE packet from the server.
>
> Teach the grub_net_process_dhcp() function to deal with OFFER packets,
> and treat ACK packets the same es BOOTREPLY packets.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This patch has changed and you retained my RB and not explained why it
has changed. Could you tell us what has happened? Next time please drop
RB if you change the code significantly.
Hmmm... It seems to me that at least partially this is due to change in
patch #5.
Daniel
> ---
> grub-core/net/bootp.c | 78 +++++++++++++++++++++++++++++++++++--------
> include/grub/net.h | 5 +++
> 2 files changed, 70 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index bfde6ef10..d0482a7d5 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -30,6 +30,21 @@ enum
> GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
> GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
> };
> +enum
> +{
> + GRUB_DHCP_MESSAGE_UNKNOWN,
> + GRUB_DHCP_MESSAGE_DISCOVER,
> + GRUB_DHCP_MESSAGE_OFFER,
> + GRUB_DHCP_MESSAGE_REQUEST,
> + GRUB_DHCP_MESSAGE_DECLINE,
> + GRUB_DHCP_MESSAGE_ACK,
> + GRUB_DHCP_MESSAGE_NAK,
> + GRUB_DHCP_MESSAGE_RELEASE,
> + GRUB_DHCP_MESSAGE_INFORM,
> +};
> +
> +/* Max timeout when waiting for BOOTP/DHCP reply */
> +#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
>
> #define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
>
> @@ -443,22 +458,59 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
> struct grub_net_card *card = iface->card;
> const struct grub_net_bootp_packet *bp = (const struct grub_net_bootp_packet *) nb->data;
> grub_size_t size = nb->tail - nb->data;
> + const grub_uint8_t *opt;
> + grub_uint8_t opt_len, type;
> + grub_uint32_t srv_id = 0;
> +
> + opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_MESSAGE_TYPE, &opt_len);
> + if (opt && opt_len == 1)
> + type = *opt;
> + else
> + type = GRUB_DHCP_MESSAGE_UNKNOWN;
> +
> + opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_SERVER_IDENTIFIER, &opt_len);
> + if (opt && opt_len == sizeof (srv_id))
> + srv_id = grub_get_unaligned32 (opt);
>
> - name = grub_xasprintf ("%s:dhcp", card->name);
> - if (!name)
> + /*
> + * If we received BOOTP reply or DHCPACK, proceed with configuration.
> + * Otherwise store offered address and server id for later processing
> + * of DHCPACK.
> + * xid and srv_id are stored in network order so do not need conversion.
> + */
> + if ((!iface->srv_id && type == GRUB_DHCP_MESSAGE_UNKNOWN)
> + || (iface->srv_id && type == GRUB_DHCP_MESSAGE_ACK
> + && bp->ident == iface->xid
> + && srv_id == iface->srv_id))
> {
> - grub_print_error ();
> - return;
> + name = grub_xasprintf ("%s:dhcp", card->name);
> + if (!name)
> + {
> + grub_print_error ();
> + return;
> + }
> + grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
> + grub_free (name);
> + if (grub_errno)
> + grub_print_error ();
> + else
> + grub_net_network_level_interface_unregister (iface);
> + }
> + else if (!iface->srv_id && type == GRUB_DHCP_MESSAGE_OFFER && srv_id)
> + {
> + iface->srv_id = srv_id;
> + iface->my_ip = bp->your_ip;
> + /* Reset retransmission timer */
> + iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
> + }
> + else if (iface->srv_id && type == GRUB_DHCP_MESSAGE_NAK
> + && bp->ident == iface->xid
> + && srv_id == iface->srv_id)
> + {
> + iface->xid = iface->srv_id = iface->my_ip = 0;
> + /* Reset retransmission timer */
> + iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
> }
> - grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
> - grub_free (name);
> - if (grub_errno)
> - grub_print_error ();
> - else
> - if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 0 &&
> - grub_memcmp (iface->name + grub_strlen (card->name),
> - ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
> - grub_net_network_level_interface_unregister (iface);
> }
>
> static char
> diff --git a/include/grub/net.h b/include/grub/net.h
> index f7b546446..68a9f1311 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -292,6 +292,9 @@ struct grub_net_network_level_interface
> struct grub_net_bootp_packet *dhcp_ack;
> grub_size_t dhcp_acklen;
> grub_uint16_t vlantag;
> + grub_uint32_t xid; /* DHCPv4 transaction id */
> + grub_uint32_t srv_id; /* DHCPv4 server_identifier */
> + grub_uint32_t my_ip; /* DHCPv4 offered IP address */
> unsigned dhcp_tmo_left; /* DHCPv4 running retransmission timeout */
> unsigned dhcp_tmo; /* DHCPv4 current retransmission timeout */
> void *data;
> @@ -460,6 +463,8 @@ enum
> GRUB_NET_BOOTP_ROOT_PATH = 0x11,
> GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
> GRUB_NET_DHCP_OVERLOAD = 52,
> + GRUB_NET_DHCP_MESSAGE_TYPE = 53,
> + GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
> GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
> GRUB_NET_DHCP_BOOTFILE_NAME = 67,
> GRUB_NET_BOOTP_END = 0xff
> --
> 2.17.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 03/10] net: dhcp: Allow overloading legacy bootfile and name field
2019-03-08 11:51 ` Andre Przywara
@ 2019-03-08 12:04 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2019-03-08 12:04 UTC (permalink / raw)
To: Andre Przywara
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Fri, Mar 08, 2019 at 11:51:27AM +0000, Andre Przywara wrote:
> On Fri, 8 Mar 2019 12:34:18 +0100
> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > On Thu, Mar 07, 2019 at 03:14:09PM +0000, Andre Przywara wrote:
> > > From: Andrei Borzenkov <arvidjaar@gmail.com>
> > >
> > > DHCP specifies a special dummy option OVERLOAD, to allow DHCP options to
> > > spill over into the (legacy) BOOTFILE and SNAME fields.
> > >
> > > Parse and handle this option properly.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >
> > In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> but one
> > nitpick below...
> >
> > > ---
> > > grub-core/net/bootp.c | 59 ++++++++++++++++++++++++++++++++++++++++++-
> > > include/grub/net.h | 1 +
> > > 2 files changed, 59 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > > index 8d6763689..860d9c80d 100644
> > > --- a/grub-core/net/bootp.c
> > > +++ b/grub-core/net/bootp.c
> > > @@ -25,11 +25,19 @@
> > > #include <grub/net/udp.h>
> > > #include <grub/datetime.h>
> > >
> > > +enum
> > > +{
> > > + GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
> > > + GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
> > > +};
> > > +
> > > static const void *
> > > find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> > > grub_uint8_t opt_code, grub_uint8_t *opt_len)
> > > {
> > > const grub_uint8_t *ptr;
> > > + grub_uint8_t overload = 0;
> > > + int end = 0;
> > > grub_size_t i;
> > >
> > > if (opt_len)
> > > @@ -54,6 +62,7 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> > > size -= sizeof (*bp);
> > > i = sizeof (grub_uint32_t);
> > >
> > > +again:
> > > while (i < size)
> > > {
> > > grub_uint8_t tagtype;
> > > @@ -67,7 +76,10 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> > >
> > > /* End tag. */
> > > if (tagtype == GRUB_NET_BOOTP_END)
> > > - break;
> > > + {
> > > + end = 1;
> > > + break;
> > > + }
> > >
> > > if (i >= size)
> > > return NULL;
> > > @@ -84,9 +96,54 @@ find_dhcp_option (const struct grub_net_bootp_packet *bp, grub_size_t size,
> > > return &ptr[i];
> > > }
> > >
> > > + if (tagtype == GRUB_NET_DHCP_OVERLOAD && taglength == 1)
> > > + overload = ptr[i];
> > > +
> > > i += taglength;
> > > }
> > >
> > > + if (!end)
> > > + return NULL;
> > > +
> > > + /* RFC2131, 4.1, 23ff:
> > > + * If the options in a DHCP message extend into the 'sname' and 'file'
> > > + * fields, the 'option overload' option MUST appear in the 'options'
> > > + * field, with value 1, 2 or 3, as specified in RFC 1533. If the
> > > + * 'option overload' option is present in the 'options' field, the
> > > + * options in the 'options' field MUST be terminated by an 'end' option,
> > > + * and MAY contain one or more 'pad' options to fill the options field.
> > > + * The options in the 'sname' and 'file' fields (if in use as indicated
> > > + * by the 'options overload' option) MUST begin with the first octet of
> > > + * the field, MUST be terminated by an 'end' option, and MUST be
> > > + * followed by 'pad' options to fill the remainder of the field. Any
> > > + * individual option in the 'options', 'sname' and 'file' fields MUST be
> > > + * entirely contained in that field. The options in the 'options' field
> > > + * MUST be interpreted first, so that any 'option overload' options may
> > > + * be interpreted. The 'file' field MUST be interpreted next (if the
> > > + * 'option overload' option indicates that the 'file' field contains
> > > + * DHCP options), followed by the 'sname' field.
> > > + *
> > > + * FIXME: We do not explicitly check for trailing 'pad' options here.
> > > + */
> > > + end = 0;
> >
> > It seems to me that this line is not needed... I can drop it before
> > committing the patch if you are OK with it.
>
> I was eyeballing this line as well, but I think it *is* needed:
> It resets end to 0, before possibly jumping back to the again label, inside the if conditions below.
> Does that make sense?
Ahhh... Right, I have missed that. So,
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
> Cheers,
> Andre.
>
> >
> > > + if (overload & GRUB_DHCP_OPT_OVERLOAD_FILE)
> > > + {
> > > + overload &= ~GRUB_DHCP_OPT_OVERLOAD_FILE;
> > > + ptr = (grub_uint8_t *) &bp->boot_file[0];
> > > + size = sizeof (bp->boot_file);
> > > + i = 0;
> > > + goto again;
> > > + }
> > > +
> > > + if (overload & GRUB_DHCP_OPT_OVERLOAD_SNAME)
> > > + {
> > > + overload &= ~GRUB_DHCP_OPT_OVERLOAD_SNAME;
> > > + ptr = (grub_uint8_t *) &bp->server_name[0];
> > > + size = sizeof (bp->server_name);
> > > + i = 0;
> > > + goto again;
> > > + }
> > > +
> > > return NULL;
> > > }
> >
> > Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets
2019-03-08 12:01 ` Daniel Kiper
@ 2019-03-08 12:21 ` Andre Przywara
2019-03-08 16:02 ` Daniel Kiper
0 siblings, 1 reply; 21+ messages in thread
From: Andre Przywara @ 2019-03-08 12:21 UTC (permalink / raw)
To: Daniel Kiper
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Fri, 8 Mar 2019 13:01:33 +0100
Daniel Kiper <daniel.kiper@oracle.com> wrote:
> On Thu, Mar 07, 2019 at 03:14:14PM +0000, Andre Przywara wrote:
> > From: Andrei Borzenkov <arvidjaar@gmail.com>
> >
> > In respone to a BOOTREQUEST packet a BOOTP server would answer with a
> > BOOTREPLY packet, which ends the conversation for good.
> > DHCP uses a 4-way handshake, where the initial server respone is an OFFER,
> > which has to be answered with REQUEST by the client again, only to be
> > completed by an ACKNOWLEDGE packet from the server.
> >
> > Teach the grub_net_process_dhcp() function to deal with OFFER packets,
> > and treat ACK packets the same es BOOTREPLY packets.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>
> This patch has changed and you retained my RB and not explained why it
> has changed. Could you tell us what has happened? Next time please drop
> RB if you change the code significantly.
Mmh, weird. I didn't touch this patch at all, that's why just added your
R-B to the commit message. I think what happened is that the subtle change
in patch 05/10 (just removing the not needed brackets, as per your comment
on v2 4/9), caused the *diff* to be vastly different. I remember fixing it
up in the "git rebase -i" process. I just did a:
$ diff -u <(git show dhcp-v2~2:grub-core/net/bootp.c) <(git show dhcp-v3~2:grub-core/net/bootp.c)
and that showed only unrelated changes.
So the resulting code change is actually identical, it's just that diff
took a different approach at expressing that.
Sorry for the confusion, hope that your R-b: still stands.
Cheers,
Andre.
> Hmmm... It seems to me that at least partially this is due to change in
> patch #5.
>
> Daniel
>
> > ---
> > grub-core/net/bootp.c | 78 +++++++++++++++++++++++++++++++++++--------
> > include/grub/net.h | 5 +++
> > 2 files changed, 70 insertions(+), 13 deletions(-)
> >
> > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> > index bfde6ef10..d0482a7d5 100644
> > --- a/grub-core/net/bootp.c
> > +++ b/grub-core/net/bootp.c
> > @@ -30,6 +30,21 @@ enum
> > GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
> > GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
> > };
> > +enum
> > +{
> > + GRUB_DHCP_MESSAGE_UNKNOWN,
> > + GRUB_DHCP_MESSAGE_DISCOVER,
> > + GRUB_DHCP_MESSAGE_OFFER,
> > + GRUB_DHCP_MESSAGE_REQUEST,
> > + GRUB_DHCP_MESSAGE_DECLINE,
> > + GRUB_DHCP_MESSAGE_ACK,
> > + GRUB_DHCP_MESSAGE_NAK,
> > + GRUB_DHCP_MESSAGE_RELEASE,
> > + GRUB_DHCP_MESSAGE_INFORM,
> > +};
> > +
> > +/* Max timeout when waiting for BOOTP/DHCP reply */
> > +#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
> >
> > #define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
> >
> > @@ -443,22 +458,59 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
> > struct grub_net_card *card = iface->card;
> > const struct grub_net_bootp_packet *bp = (const struct grub_net_bootp_packet *) nb->data;
> > grub_size_t size = nb->tail - nb->data;
> > + const grub_uint8_t *opt;
> > + grub_uint8_t opt_len, type;
> > + grub_uint32_t srv_id = 0;
> > +
> > + opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_MESSAGE_TYPE, &opt_len);
> > + if (opt && opt_len == 1)
> > + type = *opt;
> > + else
> > + type = GRUB_DHCP_MESSAGE_UNKNOWN;
> > +
> > + opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_SERVER_IDENTIFIER, &opt_len);
> > + if (opt && opt_len == sizeof (srv_id))
> > + srv_id = grub_get_unaligned32 (opt);
> >
> > - name = grub_xasprintf ("%s:dhcp", card->name);
> > - if (!name)
> > + /*
> > + * If we received BOOTP reply or DHCPACK, proceed with configuration.
> > + * Otherwise store offered address and server id for later processing
> > + * of DHCPACK.
> > + * xid and srv_id are stored in network order so do not need conversion.
> > + */
> > + if ((!iface->srv_id && type == GRUB_DHCP_MESSAGE_UNKNOWN)
> > + || (iface->srv_id && type == GRUB_DHCP_MESSAGE_ACK
> > + && bp->ident == iface->xid
> > + && srv_id == iface->srv_id))
> > {
> > - grub_print_error ();
> > - return;
> > + name = grub_xasprintf ("%s:dhcp", card->name);
> > + if (!name)
> > + {
> > + grub_print_error ();
> > + return;
> > + }
> > + grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
> > + grub_free (name);
> > + if (grub_errno)
> > + grub_print_error ();
> > + else
> > + grub_net_network_level_interface_unregister (iface);
> > + }
> > + else if (!iface->srv_id && type == GRUB_DHCP_MESSAGE_OFFER && srv_id)
> > + {
> > + iface->srv_id = srv_id;
> > + iface->my_ip = bp->your_ip;
> > + /* Reset retransmission timer */
> > + iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
> > + }
> > + else if (iface->srv_id && type == GRUB_DHCP_MESSAGE_NAK
> > + && bp->ident == iface->xid
> > + && srv_id == iface->srv_id)
> > + {
> > + iface->xid = iface->srv_id = iface->my_ip = 0;
> > + /* Reset retransmission timer */
> > + iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
> > }
> > - grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
> > - grub_free (name);
> > - if (grub_errno)
> > - grub_print_error ();
> > - else
> > - if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 0 &&
> > - grub_memcmp (iface->name + grub_strlen (card->name),
> > - ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
> > - grub_net_network_level_interface_unregister (iface);
> > }
> >
> > static char
> > diff --git a/include/grub/net.h b/include/grub/net.h
> > index f7b546446..68a9f1311 100644
> > --- a/include/grub/net.h
> > +++ b/include/grub/net.h
> > @@ -292,6 +292,9 @@ struct grub_net_network_level_interface
> > struct grub_net_bootp_packet *dhcp_ack;
> > grub_size_t dhcp_acklen;
> > grub_uint16_t vlantag;
> > + grub_uint32_t xid; /* DHCPv4 transaction id */
> > + grub_uint32_t srv_id; /* DHCPv4 server_identifier */
> > + grub_uint32_t my_ip; /* DHCPv4 offered IP address */
> > unsigned dhcp_tmo_left; /* DHCPv4 running retransmission timeout */
> > unsigned dhcp_tmo; /* DHCPv4 current retransmission timeout */
> > void *data;
> > @@ -460,6 +463,8 @@ enum
> > GRUB_NET_BOOTP_ROOT_PATH = 0x11,
> > GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
> > GRUB_NET_DHCP_OVERLOAD = 52,
> > + GRUB_NET_DHCP_MESSAGE_TYPE = 53,
> > + GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
> > GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
> > GRUB_NET_DHCP_BOOTFILE_NAME = 67,
> > GRUB_NET_BOOTP_END = 0xff
> > --
> > 2.17.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets
2019-03-08 12:21 ` Andre Przywara
@ 2019-03-08 16:02 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2019-03-08 16:02 UTC (permalink / raw)
To: Andre Przywara
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Fri, Mar 08, 2019 at 12:21:20PM +0000, Andre Przywara wrote:
> On Fri, 8 Mar 2019 13:01:33 +0100
> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > On Thu, Mar 07, 2019 at 03:14:14PM +0000, Andre Przywara wrote:
> > > From: Andrei Borzenkov <arvidjaar@gmail.com>
> > >
> > > In respone to a BOOTREQUEST packet a BOOTP server would answer with a
> > > BOOTREPLY packet, which ends the conversation for good.
> > > DHCP uses a 4-way handshake, where the initial server respone is an OFFER,
> > > which has to be answered with REQUEST by the client again, only to be
> > > completed by an ACKNOWLEDGE packet from the server.
> > >
> > > Teach the grub_net_process_dhcp() function to deal with OFFER packets,
> > > and treat ACK packets the same es BOOTREPLY packets.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >
> > This patch has changed and you retained my RB and not explained why it
> > has changed. Could you tell us what has happened? Next time please drop
> > RB if you change the code significantly.
>
> Mmh, weird. I didn't touch this patch at all, that's why just added your
> R-B to the commit message. I think what happened is that the subtle change
> in patch 05/10 (just removing the not needed brackets, as per your comment
> on v2 4/9), caused the *diff* to be vastly different. I remember fixing it
> up in the "git rebase -i" process. I just did a:
> $ diff -u <(git show dhcp-v2~2:grub-core/net/bootp.c) <(git show dhcp-v3~2:grub-core/net/bootp.c)
> and that showed only unrelated changes.
> So the resulting code change is actually identical, it's just that diff
> took a different approach at expressing that.
>
> Sorry for the confusion, hope that your R-b: still stands.
Gosh... I made a mistake when I have been comparing the patches. My fault.
So, I am taking back my rant. Sorry about that. R-b: still stands.
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 00/10] net: bootp: add native DHCPv4 support
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
` (9 preceding siblings ...)
2019-03-07 15:14 ` [PATCH v3 10/10] net: dhcp: add explicit net_dhcp command Andre Przywara
@ 2019-03-08 16:03 ` Daniel Kiper
2019-03-08 18:07 ` Andre Przywara
10 siblings, 1 reply; 21+ messages in thread
From: Daniel Kiper @ 2019-03-08 16:03 UTC (permalink / raw)
To: Andre Przywara
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Thu, Mar 07, 2019 at 03:14:06PM +0000, Andre Przywara wrote:
> A minor rework compared to v2, addressing Daniel's comments on the list.
> The former patch 2/9 has been split up to first refactor the
> parse_dhcp_vendor() function, then later introduce the OVERLOAD
> functionality. The other minor nits should be fixed as well, also I
> added Daniel's Reviewed-by: tags.
> Again tested against a modern DHCP-only server and some legacy bootpd
> version.
Patchset LGTM. If there are no objections I will apply it next week.
Thank you for doing the work!
Have a nice weekend,
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 00/10] net: bootp: add native DHCPv4 support
2019-03-08 16:03 ` [PATCH v3 00/10] net: bootp: add native DHCPv4 support Daniel Kiper
@ 2019-03-08 18:07 ` Andre Przywara
0 siblings, 0 replies; 21+ messages in thread
From: Andre Przywara @ 2019-03-08 18:07 UTC (permalink / raw)
To: Daniel Kiper
Cc: Daniel Kiper, Vladimir Serbinenko, Andrei Borzenkov,
Mark Rutland, grub-devel
On Fri, 8 Mar 2019 17:03:34 +0100
Daniel Kiper <daniel.kiper@oracle.com> wrote:
Hi Daniel,
> On Thu, Mar 07, 2019 at 03:14:06PM +0000, Andre Przywara wrote:
> > A minor rework compared to v2, addressing Daniel's comments on the list.
> > The former patch 2/9 has been split up to first refactor the
> > parse_dhcp_vendor() function, then later introduce the OVERLOAD
> > functionality. The other minor nits should be fixed as well, also I
> > added Daniel's Reviewed-by: tags.
> > Again tested against a modern DHCP-only server and some legacy bootpd
> > version.
>
> Patchset LGTM. If there are no objections I will apply it next week.
>
> Thank you for doing the work!
Many thanks to you for diligently reviewing the patches!
Looking forward to my next week's git pull ;-)
Cheers,
Andre.
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-03-08 18:07 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-07 15:14 [PATCH v3 00/10] net: bootp: add native DHCPv4 support Andre Przywara
2019-03-07 15:14 ` [PATCH v3 01/10] net: dhcp: remove dead code Andre Przywara
2019-03-07 15:14 ` [PATCH v3 02/10] net: dhcp: replace parse_dhcp_vendor() with find_dhcp_option() Andre Przywara
2019-03-08 11:29 ` Daniel Kiper
2019-03-07 15:14 ` [PATCH v3 03/10] net: dhcp: Allow overloading legacy bootfile and name field Andre Przywara
2019-03-08 11:34 ` Daniel Kiper
2019-03-08 11:51 ` Andre Przywara
2019-03-08 12:04 ` Daniel Kiper
2019-03-07 15:14 ` [PATCH v3 04/10] net: dhcp: refactor DHCP packet transmission into separate function Andre Przywara
2019-03-07 15:14 ` [PATCH v3 05/10] net: dhcp: make grub_net_process_dhcp take an interface Andre Przywara
2019-03-07 15:14 ` [PATCH v3 06/10] net: dhcp: introduce per-interface timeout Andre Przywara
2019-03-07 15:14 ` [PATCH v3 07/10] net: dhcp: use DHCP options for name and bootfile Andre Przywara
2019-03-08 11:48 ` Daniel Kiper
2019-03-07 15:14 ` [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets Andre Przywara
2019-03-08 12:01 ` Daniel Kiper
2019-03-08 12:21 ` Andre Przywara
2019-03-08 16:02 ` Daniel Kiper
2019-03-07 15:14 ` [PATCH v3 09/10] net: dhcp: actually send out DHCPv4 DISCOVER and REQUEST messages Andre Przywara
2019-03-07 15:14 ` [PATCH v3 10/10] net: dhcp: add explicit net_dhcp command Andre Przywara
2019-03-08 16:03 ` [PATCH v3 00/10] net: bootp: add native DHCPv4 support Daniel Kiper
2019-03-08 18:07 ` Andre Przywara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.