[PATCH v2 1/3] net: dhcp6: Add DHCPv6 (DHCP for IPv6)
Sean Edmond
seanedmond at linux.microsoft.com
Wed May 3 00:11:35 CEST 2023
On 2023-04-25 12:03 p.m., Ramon Fried wrote:
> On Fri, Apr 7, 2023 at 9:55 PM Simon Glass <sjg at chromium.org> wrote:
>> 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
> Nicely done, please address Simon comments and I'll accept.
All of Simon's comments were addressed in v3 of the patch series. Let
me know if you need me to add more detail.
More information about the U-Boot
mailing list