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
