On Fri, Apr 7, 2023 at 9:55 PM Simon Glass <s...@chromium.org> wrote:
>
> Hi,
>
> On Fri, 7 Apr 2023 at 18:56, <seanedm...@linux.microsoft.com> wrote:
> >
> > From: Sean Edmond <seanedm...@microsoft.com>
> >
> > Adds DHCPv6 protocol to u-boot.
> >
> > Allows for address assignement with DHCPv6 4-message exchange
> > (SOLICIT->ADVERTISE->REQUEST->REPLY). Includes DHCPv6 options
> > required by RFC 8415. Also adds DHCPv6 options required
> > for PXE boot.
> >
> > Possible enhancements:
> > - Duplicate address detection on DHCPv6 assigned address
> > - IPv6 address assignement through SLAAC
> > - Sending/parsing other DHCPv6 options (NTP, DNS, etc...)
> >
> > Signed-off-by: Sean Edmond <seanedm...@microsoft.com>
> > ---
> > include/net.h | 8 +-
> > net/Makefile | 1 +
> > net/dhcpv6.c | 735 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > net/dhcpv6.h | 212 +++++++++++++++
> > net/net.c | 12 +
> > 5 files changed, 966 insertions(+), 2 deletions(-)
> > create mode 100644 net/dhcpv6.c
> > create mode 100644 net/dhcpv6.h
>
> This looks good to me. I just have a few nits below. With those fixed:
>
> Reviewed-by: Simon Glass <s...@chromium.org>
>
> >
> > diff --git a/include/net.h b/include/net.h
> > index 399af5e064..cac818e292 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -484,6 +484,10 @@ extern char net_hostname[32]; /* Our
> > hostname */
> > #ifdef CONFIG_NET
> > extern char net_root_path[CONFIG_BOOTP_MAX_ROOT_PATH_LEN]; /* Our root
> > path */
> > #endif
> > +#if defined(CONFIG_DHCP6_PXE_DHCP_OPTION)
>
> You can drop this #ifdef as any reference to a non-existent var will
> give a build error.
>
> > +/* Indicates whether the pxe path prefix / config file was specified in
> > dhcp option */
> > +extern char *pxelinux_configfile;
> > +#endif
> > /** END OF BOOTP EXTENTIONS **/
> > extern u8 net_ethaddr[ARP_HLEN]; /* Our ethernet
> > address */
> > extern u8 net_server_ethaddr[ARP_HLEN]; /* Boot server enet
> > address */
> > @@ -504,8 +508,8 @@ extern ushort net_native_vlan; /*
> > Our Native VLAN */
> > extern int net_restart_wrap; /* Tried all network
> > devices */
> >
> > enum proto_t {
> > - BOOTP, RARP, ARP, TFTPGET, DHCP, PING, PING6, DNS, NFS, CDP,
> > NETCONS,
> > - SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL, UDP, NCSI, WGET
> > + BOOTP, RARP, ARP, TFTPGET, DHCP, DHCP6, PING, PING6, DNS, NFS, CDP,
> > + NETCONS, SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL, UDP,
> > NCSI, WGET
> > };
> >
> > extern char net_boot_file_name[1024];/* Boot File name */
> > diff --git a/net/Makefile b/net/Makefile
> > index bea000b206..5968110170 100644
> > --- a/net/Makefile
> > +++ b/net/Makefile
> > @@ -22,6 +22,7 @@ obj-$(CONFIG_IPV6) += net6.o
> > obj-$(CONFIG_CMD_NFS) += nfs.o
> > obj-$(CONFIG_CMD_PING) += ping.o
> > obj-$(CONFIG_CMD_PING6) += ping6.o
> > +obj-$(CONFIG_CMD_DHCP6) += dhcpv6.o
> > obj-$(CONFIG_CMD_PCAP) += pcap.o
> > obj-$(CONFIG_CMD_RARP) += rarp.o
> > obj-$(CONFIG_CMD_SNTP) += sntp.o
> > diff --git a/net/dhcpv6.c b/net/dhcpv6.c
> > new file mode 100644
> > index 0000000000..9204909c1f
> > --- /dev/null
> > +++ b/net/dhcpv6.c
> > @@ -0,0 +1,735 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) Microsoft Corporation
> > + * Author: Sean Edmond <seanedm...@microsoft.com>
> > + *
> > + */
> > +
> > +/* Simple DHCP6 network layer implementation. */
> > +
> > +#include <common.h>
> > +#include <bootstage.h>
> > +#include <command.h>
> > +#include <env.h>
> > +#include <efi_loader.h>
> > +#include <log.h>
> > +#include <net.h>
> > +#include <rand.h>
> > +#include <uuid.h>
> > +#include <linux/delay.h>
> > +#include <net/tftp.h>
> > +#include "dhcpv6.h"
> > +#include <net6.h>
> > +#include <malloc.h>
> > +#include "net_rand.h"
>
> Please fix header order:
> https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files
>
> > +
> > +#define PORT_DHCP6_S 547 /* DHCP6 server UDP port */
> > +#define PORT_DHCP6_C 546 /* DHCP6 client UDP port */
> > +
> > +/* default timeout parameters (in ms) */
> > +#define SOL_MAX_DELAY_MS 1000
> > +#define SOL_TIMEOUT_MS 1000
> > +#define SOL_MAX_RT_MS 3600000
> > +#define REQ_TIMEOUT_MS 1000
> > +#define REQ_MAX_RT_MS 30000
> > +#define REQ_MAX_RC 10
> > +#define MAX_WAIT_TIME_MS 60000
> > +
> > +/* global variable to track any updates from DHCP6 server */
> > +int updated_sol_max_rt_ms = SOL_MAX_RT_MS;
> > +
> > +static void dhcp6_timeout_handler(void);
> > +static void dhcp6_state_machine(bool timeout, uchar *rx_pkt, unsigned int
> > len);
> > +static void dhcp6_parse_options(uchar *rx_pkt, unsigned int len);
>
> Rather than forward decls can you reorder the functions?
>
> > +
> > +struct dhcp6_sm_params sm_params;
> > +
> > +/*
> > + * Handle DHCP received packets (set as UDP handler)
> > + */
>
> Please check single-line comment style
>
> > +static void dhcp6_handler(uchar *pkt, unsigned int dest, struct in_addr
> > sip,
> > + unsigned int src, unsigned int len)
> > +{
> > + /* return if ports don't match DHCPv6 ports */
> > + if (dest != PORT_DHCP6_C || src != PORT_DHCP6_S)
> > + return;
> > +
> > + dhcp6_state_machine(false, pkt, len);
> > +}
> > +
> [..]
>
> > + case DHCP6_OPTION_OPT_BOOTFILE_URL:
> > + debug("DHCP6_OPTION_OPT_BOOTFILE_URL FOUND\n");
> > + copy_filename(net_boot_file_name, option_ptr,
> > option_len + 1);
> > + debug("net_boot_file_name: %s\n",
> > net_boot_file_name);
> > +
> > + /* copy server_ip6 (required for PXE) */
> > + s = strchr(net_boot_file_name, '[');
> > + e = strchr(net_boot_file_name, ']');
> > + if (s && e && e > s)
> > + string_to_ip6(s + 1, e - s - 1,
> > &net_server_ip6);
> > + break;
> > +#if IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION)
> > + case DHCP6_OPTION_OPT_BOOTFILE_PARAM:
>
> Can you do something like this to avoid the #ifdef ?
>
> case DHCP6_OPTION_OPT_BOOTFILE_PARAM:
> if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) {
> ...
> }
>
> You could even add a 'fallthough' if you move it to last position in
> the switch().
>
> > + debug("DHCP6_OPTION_OPT_BOOTFILE_PARAM FOUND\n");
> > +
> > + if (pxelinux_configfile)
> > + free(pxelinux_configfile);
> > +
> > + pxelinux_configfile = (char *)malloc((option_len +
> > 1) * sizeof(char));
> > + if (pxelinux_configfile) {
> > + memcpy(pxelinux_configfile, option_ptr,
> > option_len);
> > + pxelinux_configfile[option_len] = '\0';
>
> Does strlcpy() with option_len + 1 work here?
>
> > + }
>
> If malloc() fails it needs to be reported.
>
> > +
> > + debug("PXE CONFIG FILE %s\n", pxelinux_configfile);
> > + break;
> > +#endif
> > + case DHCP6_OPTION_PREFERENCE:
> > + debug("DHCP6_OPTION_PREFERENCE FOUND\n");
> > + sm_params.rx_status.preference = *option_ptr;
> > + break;
> > + default:
> > + debug("Unknown Option ID: %d, skipping parsing\n",
> > + ntohs(option_hdr->option_id));
> > + break;
> > + }
> > + /* Increment to next option header */
> > + option_hdr = (struct dhcp6_option_hdr *)(((uchar
> > *)option_hdr) +
> > + sizeof(struct dhcp6_option_hdr) + option_len);
> > + }
> > +}
> > +
>
> [..]
>
> > +/*
> > + * Timeout for DHCP6 SOLICIT/REQUEST.
>
> Fix single-line comment format (globally)
>
> > + */
> > +static void dhcp6_timeout_handler(void)
> > +{
> > + /* call state machine with the timeout flag */
> > + dhcp6_state_machine(true, NULL, 0);
> > +}
> > +
> > +/*
> > + * Start or restart DHCP6
> > + */
> > +void dhcp6_start(void)
> > +{
> > + memset(&sm_params, 0, sizeof(struct dhcp6_sm_params));
> > +
> > + /* seed the RNG with MAC address */
> > + srand_mac();
> > +
> > + sm_params.curr_state = DHCP6_INIT;
> > + dhcp6_state_machine(false, NULL, 0);
> > +}
> > diff --git a/net/dhcpv6.h b/net/dhcpv6.h
> > new file mode 100644
> > index 0000000000..6a0158127a
> > --- /dev/null
> > +++ b/net/dhcpv6.h
> > @@ -0,0 +1,212 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) Microsoft Corporation
> > + * Author: Sean Edmond <seanedm...@microsoft.com>
> > + *
> > + */
> > +
> > +#ifndef __DHCP6_H__
> > +#define __DHCP6_H__
> > +
> > +#include <net6.h>
>
> Is this needed? If so it might be better to split out the bits you
> need into a net6_defs.h or something like that.
>
> > +#include <net.h>
>
> Please don't include net.h as it brings in heaps of stuff.
>
> > +
> > +/* Message types */
> > +#define DHCP6_MSG_SOLICIT 1
> > +#define DHCP6_MSG_ADVERTISE 2
> > +#define DHCP6_MSG_REQUEST 3
> > +#define DHCP6_MSG_REPLY 7
>
> [..]
>
> > +
> > +/* Send a DHCPv6 request */
>
> Can you add a bit more detail here, e.g. mention the state machine and
> what happens next?
>
> > +void dhcp6_start(void);
> > +
> > +#endif /* __DHCP6_H__ */
> > diff --git a/net/net.c b/net/net.c
> > index c9a749f6cc..73d5b2bc80 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -122,6 +122,9 @@
> > #endif
> > #include <net/tcp.h>
> > #include <net/wget.h>
> > +#if defined(CONFIG_CMD_DHCP6)
> > +#include "dhcpv6.h"
>
> No need to exclude the header so you can drop the #ifdef
>
> > +#endif
> >
> > /** BOOTP EXTENTIONS **/
> >
> > @@ -135,6 +138,10 @@ struct in_addr net_dns_server;
> > /* Our 2nd DNS IP address */
> > struct in_addr net_dns_server2;
> > #endif
> > +#if defined(CONFIG_DHCP6_PXE_DHCP_OPTION)
> > +/* Indicates whether the pxe path prefix / config file was specified in
> > dhcp option */
> > +char *pxelinux_configfile;
> > +#endif
> >
> > /** END OF BOOTP EXTENTIONS **/
> >
> > @@ -510,6 +517,11 @@ restart:
> > dhcp_request(); /* Basically same as BOOTP
> > */
> > break;
> > #endif
> > +#if defined(CONFIG_CMD_DHCP6)
> > + case DHCP6:
>
> Can we use the if (IS_ENABLED()) thing again here to drop this #ifdef?
>
> > + dhcp6_start();
> > + break;
> > +#endif
> > #if defined(CONFIG_CMD_BOOTP)
> > case BOOTP:
> > bootp_reset();
> > --
> > 2.40.0
> >
>
> Regards,
> Simon
Nicely done, please address Simon comments and I'll accept.
