[PATCH v2 1/3] net: dhcp6: Add DHCPv6 (DHCP for IPv6)

Simon Glass sjg at chromium.org
Fri Apr 7 20:55:17 CEST 2023


Hi,

On Fri, 7 Apr 2023 at 18:56, <seanedmond at linux.microsoft.com> wrote:
>
> From: Sean Edmond <seanedmond at 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 <seanedmond at 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 <sjg at 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 <seanedmond at 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 <seanedmond at 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


More information about the U-Boot mailing list