From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1glaI8-0007ux-Ih for mharc-grub-devel@gnu.org; Mon, 21 Jan 2019 09:11:08 -0500 Received: from eggs.gnu.org ([209.51.188.92]:55319) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1glaI3-0007sr-Mz for grub-devel@gnu.org; Mon, 21 Jan 2019 09:11:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1glaHv-0000Gg-4b for grub-devel@gnu.org; Mon, 21 Jan 2019 09:11:03 -0500 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:59806 helo=foss.arm.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1glaHu-0000Fe-Rt for grub-devel@gnu.org; Mon, 21 Jan 2019 09:10:55 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id AD8C715BE; Mon, 21 Jan 2019 06:10:52 -0800 (PST) Received: from donnerap.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 79EE33F5C1; Mon, 21 Jan 2019 06:10:51 -0800 (PST) Date: Mon, 21 Jan 2019 14:10:48 +0000 From: Andre Przywara To: Daniel Kiper Cc: Vladimir Serbinenko , Andrei Borzenkov , Daniel Kiper , Mark Rutland , Subject: Re: [PATCH] bootp: add native DHCPv4 support Message-ID: <20190121141048.6f5ec78f@donnerap.cambridge.arm.com> In-Reply-To: <20190121120208.tadbamgtdl2fpztl@tomti.i.net-space.pl> References: <20190111155934.233587-1-andre.przywara@arm.com> <20190121120208.tadbamgtdl2fpztl@tomti.i.net-space.pl> Organization: ARM X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 217.140.101.70 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Jan 2019 14:11:07 -0000 On Mon, 21 Jan 2019 13:02:08 +0100 Daniel Kiper wrote: Hi Daniel, thanks very much for your reply! > On Fri, Jan 11, 2019 at 03:59:34PM +0000, Andre Przywara wrote: > > From: Andrei Borzenkov > > > > 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). > > > > I think that we should put Andrei's credits here. Probably we cannot > add SOB without his approval. However, I am happy with anything else > which does something in the similar fashion. Definitely it's his patch, and the From: line above should make this clear and the patch would end up with his authorship in git. Happy to take any other tags, but as you mentioned, I can't just put his SoB here. > > Signed-off-by: Andre Przywara > > > > --- > > Hi, > > > > this patch 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 > > It seems to me that two sentences are glued here... > > > ignored the reply. I guess this is as close to a "BOOTP only > > server" as we can get in 2019. > > I am OK with that. > > > 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. > > Could you merge your comment with the commit message above? Please > just refer to the current situation. If something historic should be > referred please say clearly that it does not apply for today. Sure. > Is it possible to split this patch to smaller pieces? Possible: yes, though I am not sure I understand enough of it and grub to do it properly. Will try my best, though, as soon as I find some spare cycles to understand the patch a bit better. > Some nitpicks below... > > > 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 > > > > grub-core/net/bootp.c | 759 > > ++++++++++++++++++++++++++++-------------- grub-core/net/ip.c > > | 2 +- include/grub/net.h | 14 +- > > 3 files changed, 528 insertions(+), 247 deletions(-) > > > > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c > > index 9e2fdb795..890df715b 100644 > > --- a/grub-core/net/bootp.c > > +++ b/grub-core/net/bootp.c > > If we add DHCP support here then I think we should change file name in > the following patch. Yeah, makes some sense. > > @@ -25,25 +25,107 @@ > > #include > > #include > > > > -static void > > -parse_dhcp_vendor (const char *name, const void *vend, int limit, > > int *mask) +struct grub_dhcp_discover_options > > + { > > Drop spaces before "{". Yes, I saw this, fixed it, then reverted it because I couldn't find an explicit coding style and didn't want to mess with Andrei's original work needlessly. Consider it fixed. The rest of the comments make sense, I will address them eventually, unless someone (Andrei?) screams... Cheers, Andre. > > + grub_uint8_t magic[4]; > > + struct > > + { > > Drop two spaces starting from above line... > > > + grub_uint8_t code; > > + grub_uint8_t len; > > + grub_uint8_t data; > > + } GRUB_PACKED message_type; > > + grub_uint8_t end; > > + } GRUB_PACKED; > > ...up to here. Same thing for structs below. > > > + > > +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 > > + { > > Drop two spaces starting from above line... > > > + 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, > > + }; > > ...up to here. > > > +enum > > + { > > + GRUB_DHCP_OPT_OVERLOAD_FILE = 1, > > + GRUB_DHCP_OPT_OVERLOAD_SNAME = 2, > > + }; > > Ditto. > > > + > > +#define GRUB_BOOTP_MAX_OPTIONS_SIZE 64 > > +#define OFFSET_OF(x, y) ((grub_size_t)((grub_uint8_t *)((y)->x) - > > (grub_uint8_t *)(y))) > > Please define this macro behind GRUB_DHCP_MAX_PACKET_TIMEOUT. > > > + > > +/* 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, > > s/grub_net_bootp_packet/grub_net_dhcp_packet/? > > And in general should not we change bootp with dhcp everywhere after > this patch? > > > + grub_uint8_t opt_code, grub_uint8_t *opt_len) > > { > > - const grub_uint8_t *ptr, *ptr0; > > + const grub_uint8_t *ptr; > > + grub_uint8_t overload = 0; > > + int end = 0; > > + grub_size_t i; > > + > > + if (opt_len) > > + *opt_len = 0; > > + > > + /* Magic */ > > + if (size < sizeof (*bp) + sizeof (grub_uint32_t)) > > + goto out; > > > > - ptr = ptr0 = vend; > > + ptr = (grub_uint8_t *) (bp + 1); > > Please add one line comment where (bp + 1) points to. > > > 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) > > + goto out; > > + > > + size -= sizeof (*bp); > > + i = sizeof (grub_uint32_t); > > + > > +again: > > + 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,84 +133,77 @@ parse_dhcp_vendor (const char *name, const > > void *vend, int limit, int *mask) > > > > /* End tag. */ > > if (tagtype == GRUB_NET_BOOTP_END) > > - return; > > + { > > + end = 1; > > + break; > > + } > > + > > + if (i >= size) > > + goto out; > > > > - taglength = *ptr++; > > + taglength = ptr[i++]; > > + if (i + taglength >= size) > > + goto out; > > > > - switch (tagtype) > > + /* FIXME RFC 3396 options concatentation */ > > + if (tagtype == opt_code) > > { > > - 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; > > + if (opt_len) > > + *opt_len = taglength; > > + return &ptr[i]; > > + } > > > > - 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: > > + if (tagtype == GRUB_NET_DHCP_OVERLOAD && taglength == 1) > > + overload = ptr[i]; > > + > > + i += taglength; > > + } > > + > > + /* 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. > > + */ > > Could you convert all multiline comments to the following format? > > /* > * 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' > ... > * > * FIXME: We do not explicitly check for trailing 'pad' options here. > */ > > > + if (end) > > + { > > + end = 0; > > + if (overload & GRUB_DHCP_OPT_OVERLOAD_FILE) > > { > > - 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; > > - } > > + overload &= ~GRUB_DHCP_OPT_OVERLOAD_FILE; > > + ptr = (grub_uint8_t *) &bp->boot_file[0]; > > + size = sizeof (bp->boot_file); > > + i = 0; > > + goto again; > > } > > - 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. */ > > - } > > > > - ptr += taglength; > > - } > > -} > > + 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; > > + } > > + } > > > > -#define OFFSET_OF(x, y) ((grub_size_t)((grub_uint8_t *)((y)->x) - > > (grub_uint8_t *)(y))) > > This is the move. I think that this should be in separate patch. > > > +out: > > + return 0; > > +} > > [...] > > > +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; > > + > > + 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); > > Please define 128 as a constant. > > > + 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); > > + 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) > > + goto out; > > + > > + pack = (void *) nb->data; > > + grub_memset (pack, 0, sizeof (*pack)); > > + pack->opcode = 1; > > + pack->hw_type = 1; > > + pack->hw_len = 6; > > Constants instead of plain numbers? Or at least comments what these > numbers mean. > > > + 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); > > + 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); > > + > > + 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); > > OK, these are port numbers but same as above. > > > + udph->chksum = 0; > > + udph->len = grub_cpu_to_be16 (nb->tail - nb->data); > > + target.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4; > > + target.ipv4 = 0xffffffff; > > Ditto. > > Daniel