[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] bootp: add native DHCPv4 support
|
From: |
Andre Przywara |
|
Subject: |
Re: [PATCH] bootp: add native DHCPv4 support |
|
Date: |
Mon, 21 Jan 2019 14:10:48 +0000 |
On Mon, 21 Jan 2019 13:02:08 +0100
Daniel Kiper <address@hidden> 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 <address@hidden>
> >
> > 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 <address@hidden>
> >
> > ---
> > 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 <grub/net/udp.h>
> > #include <grub/datetime.h>
> >
> > -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