* [PATCH v2 0/3] BOOTP/DHCPv4 enhancements
@ 2023-10-24 0:21 seanedmond
2023-10-24 0:21 ` [PATCH v2 1/3] net: Get pxe config file from dhcp option 209 seanedmond
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: seanedmond @ 2023-10-24 0:21 UTC (permalink / raw)
To: u-boot; +Cc: joe.hershberger, rfried.dev, sjg, xypron.glpk, ilias.apalodimas
From: Sean Edmond <seanedmond@microsoft.com>
In our datacenter application, a single DHCP server is servicing 36000+ clients.
Improvements are required to the DHCPv4 retransmission behavior to align with
RFC and ensure less pressure is exerted on the server:
- retransmission backoff interval maximum is configurable
(environment variable bootpretransmitperiodmax)
- initial retransmission backoff interval is configurable
(environment variable bootpretransmitperiodinit)
- transaction ID is kept the same for each BOOTP/DHCPv4 request
(not recreated on each retry)
For our application we'll use:
- bootpretransmitperiodmax=16000
- bootpretransmitperiodinit=2000
A new configuration BOOTP_RANDOM_XID has been added to enable a randomized
BOOTP/DHCPv4 transaction ID.
Add functionality for DHCPv4 sending/parsing option 209 (PXE config file).
Enabled with Kconfig BOOTP_PXE_DHCP_OPTION. Note, this patch was
submitted previously but this latest version has been enhanced to
avoid a possible double free().
changes in v2:
- use env_get_ulong() to get environment variables
Sean Edmond (3):
net: Get pxe config file from dhcp option 209
net: bootp: BOOTP/DHCPv4 retransmission improvements
net: bootp: add config option BOOTP_RANDOM_XID
cmd/Kconfig | 11 ++++++++
cmd/pxe.c | 10 +++++++
net/bootp.c | 78 ++++++++++++++++++++++++++++++++++++++++++-----------
3 files changed, 84 insertions(+), 15 deletions(-)
--
2.40.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-10-24 0:21 [PATCH v2 0/3] BOOTP/DHCPv4 enhancements seanedmond
@ 2023-10-24 0:21 ` seanedmond
2023-10-24 5:54 ` Heinrich Schuchardt
2023-10-24 0:21 ` [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements seanedmond
2023-10-24 0:21 ` [PATCH v2 3/3] net: bootp: add config option BOOTP_RANDOM_XID seanedmond
2 siblings, 1 reply; 19+ messages in thread
From: seanedmond @ 2023-10-24 0:21 UTC (permalink / raw)
To: u-boot; +Cc: joe.hershberger, rfried.dev, sjg, xypron.glpk, ilias.apalodimas
From: Sean Edmond <seanedmond@microsoft.com>
Allow dhcp server pass pxe config file full path by using option 209
Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
---
cmd/Kconfig | 4 ++++
cmd/pxe.c | 10 ++++++++++
net/bootp.c | 21 +++++++++++++++++++++
3 files changed, 35 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5bc0a92d57..adbb1a6187 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
default 0x15 if ARM
default 0x0 if X86
+config BOOTP_PXE_DHCP_OPTION
+ bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
+ depends on BOOTP_PXE
+
config BOOTP_VCI_STRING
string
depends on CMD_BOOTP
diff --git a/cmd/pxe.c b/cmd/pxe.c
index 704589702f..9404f44518 100644
--- a/cmd/pxe.c
+++ b/cmd/pxe.c
@@ -65,6 +65,8 @@ static int pxe_dhcp_option_path(struct pxe_context *ctx, unsigned long pxefile_a
int ret = get_pxe_file(ctx, pxelinux_configfile, pxefile_addr_r);
free(pxelinux_configfile);
+ /* set to NULL to avoid double-free if DHCP is tried again */
+ pxelinux_configfile = NULL;
return ret;
}
@@ -141,6 +143,14 @@ int pxe_get(ulong pxefile_addr_r, char **bootdirp, ulong *sizep, bool use_ipv6)
env_get("bootfile"), use_ipv6))
return -ENOMEM;
+ if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION) &&
+ pxelinux_configfile && !use_ipv6) {
+ if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
+ goto done;
+
+ goto error_exit;
+ }
+
if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) &&
pxelinux_configfile && use_ipv6) {
if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
diff --git a/net/bootp.c b/net/bootp.c
index 7b0f45e18a..6800290963 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -26,6 +26,7 @@
#ifdef CONFIG_BOOTP_RANDOM_DELAY
#include "net_rand.h"
#endif
+#include <malloc.h>
#define BOOTP_VENDOR_MAGIC 0x63825363 /* RFC1048 Magic Cookie */
@@ -601,6 +602,10 @@ static int dhcp_extended(u8 *e, int message_type, struct in_addr server_ip,
*e++ = 42;
*cnt += 1;
#endif
+ if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
+ *e++ = 209; /* PXELINUX Config File */
+ *cnt += 1;
+ }
/* no options, so back up to avoid sending an empty request list */
if (*cnt == 0)
e -= 2;
@@ -909,6 +914,22 @@ static void dhcp_process_options(uchar *popt, uchar *end)
net_boot_file_name[size] = 0;
}
break;
+ case 209: /* PXELINUX Config File */
+ if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
+ /* In case it has already been allocated when get DHCP Offer packet,
+ * free first to avoid memory leak.
+ */
+ if (pxelinux_configfile)
+ free(pxelinux_configfile);
+
+ pxelinux_configfile = (char *)malloc((oplen + 1) * sizeof(char));
+
+ if (pxelinux_configfile)
+ strlcpy(pxelinux_configfile, popt + 2, oplen + 1);
+ else
+ printf("Error: Failed to allocate pxelinux_configfile\n");
+ }
+ break;
default:
#if defined(CONFIG_BOOTP_VENDOREX)
if (dhcp_vendorex_proc(popt))
--
2.40.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements
2023-10-24 0:21 [PATCH v2 0/3] BOOTP/DHCPv4 enhancements seanedmond
2023-10-24 0:21 ` [PATCH v2 1/3] net: Get pxe config file from dhcp option 209 seanedmond
@ 2023-10-24 0:21 ` seanedmond
2023-10-24 6:06 ` Heinrich Schuchardt
2023-10-24 0:21 ` [PATCH v2 3/3] net: bootp: add config option BOOTP_RANDOM_XID seanedmond
2 siblings, 1 reply; 19+ messages in thread
From: seanedmond @ 2023-10-24 0:21 UTC (permalink / raw)
To: u-boot; +Cc: joe.hershberger, rfried.dev, sjg, xypron.glpk, ilias.apalodimas
From: Sean Edmond <seanedmond@microsoft.com>
This patch introduces 3 improvements to align with RFC 951:
- retransmission backoff interval maximum is configurable
- initial retranmission backoff interval is configurable
- transaction ID is kept the same for each BOOTP/DHCPv4 request
In applications where thousands of nodes are serviced by a single DHCP
server, maximizing the retransmission backoff interval at 2 seconds (the
current u-boot default) exerts high pressure on the DHCP server and
network layer.
RFC 951 “7.2. Client Retransmission Strategy” states that the
retransmission backoff interval should maximize at 60 seconds. This
patch allows the interval to be configurable using the environment
variable "bootpretransmitperiodmax"
The initial retranmission backoff period defaults to 250ms, which is
also too small for these scenarios with many clients. This patch makes
the initial retransmission interval to be configurable using the
environment variable "bootpretransmitperiodinit".
Also, on a retransmission it is not expected for the transaction ID to
change (only the 'secs' field should be updated). Let's save the
transaction ID and use the same transaction ID for each BOOTP/DHCPv4
exchange.
Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
---
net/bootp.c | 56 ++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 40 insertions(+), 16 deletions(-)
diff --git a/net/bootp.c b/net/bootp.c
index 6800290963..bab17a9ceb 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -42,6 +42,17 @@
*/
#define TIMEOUT_MS ((3 + (CONFIG_NET_RETRY_COUNT * 5)) * 1000)
+/*
+ * According to rfc951 : 7.2. Client Retransmission Strategy
+ * "After the 'average' backoff reaches about 60 seconds, it should be
+ * increased no further, but still randomized."
+ *
+ * U-Boot has saturated this backoff at 2 seconds for a long time.
+ * To modify, set the environment variable "bootpretransmitperiodmax"
+ */
+#define RETRANSMIT_PERIOD_MAX_MS 2000
+#define RETRANSMIT_PERIOD_INIT_MS 250
+
#ifndef CFG_DHCP_MIN_EXT_LEN /* minimal length of extension list */
#define CFG_DHCP_MIN_EXT_LEN 64
#endif
@@ -53,6 +64,7 @@
u32 bootp_ids[CFG_BOOTP_ID_CACHE_SIZE];
unsigned int bootp_num_ids;
int bootp_try;
+u32 bootp_id;
ulong bootp_start;
ulong bootp_timeout;
char net_nis_domain[32] = {0,}; /* Our NIS domain */
@@ -60,6 +72,7 @@ char net_hostname[32] = {0,}; /* Our hostname */
char net_root_path[CONFIG_BOOTP_MAX_ROOT_PATH_LEN] = {0,}; /* Our bootpath */
static ulong time_taken_max;
+static u32 retransmit_period_max_ms;
#if defined(CONFIG_CMD_DHCP)
static dhcp_state_t dhcp_state = INIT;
@@ -414,8 +427,8 @@ static void bootp_timeout_handler(void)
}
} else {
bootp_timeout *= 2;
- if (bootp_timeout > 2000)
- bootp_timeout = 2000;
+ if (bootp_timeout > retransmit_period_max_ms)
+ bootp_timeout = retransmit_period_max_ms;
net_set_timeout_handler(bootp_timeout, bootp_timeout_handler);
bootp_request();
}
@@ -711,10 +724,14 @@ static int bootp_extended(u8 *e)
void bootp_reset(void)
{
+ char *ep; /* Environment pointer */
+
bootp_num_ids = 0;
bootp_try = 0;
bootp_start = get_timer(0);
- bootp_timeout = 250;
+
+ bootp_timeout = env_get_ulong("bootpretransmitperiodinit", 10, RETRANSMIT_PERIOD_INIT_MS);
+
}
void bootp_request(void)
@@ -726,7 +743,6 @@ void bootp_request(void)
#ifdef CONFIG_BOOTP_RANDOM_DELAY
ulong rand_ms;
#endif
- u32 bootp_id;
struct in_addr zero_ip;
struct in_addr bcast_ip;
char *ep; /* Environment pointer */
@@ -742,6 +758,9 @@ void bootp_request(void)
else
time_taken_max = TIMEOUT_MS;
+ retransmit_period_max_ms = env_get_ulong("bootpretransmitperiodmax", 10,
+ RETRANSMIT_PERIOD_MAX_MS);
+
#ifdef CONFIG_BOOTP_RANDOM_DELAY /* Random BOOTP delay */
if (bootp_try == 0)
srand_mac();
@@ -801,18 +820,23 @@ void bootp_request(void)
extlen = bootp_extended((u8 *)bp->bp_vend);
#endif
- /*
- * Bootp ID is the lower 4 bytes of our ethernet address
- * plus the current time in ms.
- */
- bootp_id = ((u32)net_ethaddr[2] << 24)
- | ((u32)net_ethaddr[3] << 16)
- | ((u32)net_ethaddr[4] << 8)
- | (u32)net_ethaddr[5];
- bootp_id += get_timer(0);
- bootp_id = htonl(bootp_id);
- bootp_add_id(bootp_id);
- net_copy_u32(&bp->bp_id, &bootp_id);
+ /* Only generate a new transaction ID for each new BOOTP request */
+ if (bootp_try == 1) {
+ /*
+ * Bootp ID is the lower 4 bytes of our ethernet address
+ * plus the current time in ms.
+ */
+ bootp_id = ((u32)net_ethaddr[2] << 24)
+ | ((u32)net_ethaddr[3] << 16)
+ | ((u32)net_ethaddr[4] << 8)
+ | (u32)net_ethaddr[5];
+ bootp_id += get_timer(0);
+ bootp_id = htonl(bootp_id);
+ bootp_add_id(bootp_id);
+ net_copy_u32(&bp->bp_id, &bootp_id);
+ } else {
+ net_copy_u32(&bp->bp_id, &bootp_id);
+ }
/*
* Calculate proper packet lengths taking into account the
--
2.40.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 3/3] net: bootp: add config option BOOTP_RANDOM_XID
2023-10-24 0:21 [PATCH v2 0/3] BOOTP/DHCPv4 enhancements seanedmond
2023-10-24 0:21 ` [PATCH v2 1/3] net: Get pxe config file from dhcp option 209 seanedmond
2023-10-24 0:21 ` [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements seanedmond
@ 2023-10-24 0:21 ` seanedmond
2023-10-24 6:19 ` Heinrich Schuchardt
2 siblings, 1 reply; 19+ messages in thread
From: seanedmond @ 2023-10-24 0:21 UTC (permalink / raw)
To: u-boot; +Cc: joe.hershberger, rfried.dev, sjg, xypron.glpk, ilias.apalodimas
From: Sean Edmond <seanedmond@microsoft.com>
The new config option BOOTP_RANDOM_XID will randomize the transaction ID
for each new BOOT/DHCPv4 exchange.
Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
---
cmd/Kconfig | 7 +++++++
net/bootp.c | 31 +++++++++++++++++--------------
2 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig
index adbb1a6187..910f465d14 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1838,6 +1838,13 @@ config BOOTP_VCI_STRING
default "U-Boot.arm" if ARM
default "U-Boot"
+config BOOTP_RANDOM_XID
+ bool "Send random transaction ID to BOOTP/DHCP server"
+ depends on CMD_BOOTP
+ help
+ Selecting this will allow for a random transaction ID to get
+ selected for each BOOTP/DHCPv4 exchange.
+
if CMD_DHCP6
config DHCP6_PXE_CLIENTARCH
diff --git a/net/bootp.c b/net/bootp.c
index bab17a9ceb..f1ca2efacf 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -822,22 +822,25 @@ void bootp_request(void)
/* Only generate a new transaction ID for each new BOOTP request */
if (bootp_try == 1) {
- /*
- * Bootp ID is the lower 4 bytes of our ethernet address
- * plus the current time in ms.
- */
- bootp_id = ((u32)net_ethaddr[2] << 24)
- | ((u32)net_ethaddr[3] << 16)
- | ((u32)net_ethaddr[4] << 8)
- | (u32)net_ethaddr[5];
- bootp_id += get_timer(0);
- bootp_id = htonl(bootp_id);
- bootp_add_id(bootp_id);
- net_copy_u32(&bp->bp_id, &bootp_id);
- } else {
- net_copy_u32(&bp->bp_id, &bootp_id);
+ if (IS_ENABLED(CONFIG_BOOTP_RANDOM_XID)) {
+ srand(get_ticks() + rand());
+ bootp_id = rand();
+ } else {
+ /*
+ * Bootp ID is the lower 4 bytes of our ethernet address
+ * plus the current time in ms.
+ */
+ bootp_id = ((u32)net_ethaddr[2] << 24)
+ | ((u32)net_ethaddr[3] << 16)
+ | ((u32)net_ethaddr[4] << 8)
+ | (u32)net_ethaddr[5];
+ bootp_id += get_timer(0);
+ bootp_id = htonl(bootp_id);
+ }
}
+ bootp_add_id(bootp_id);
+ net_copy_u32(&bp->bp_id, &bootp_id);
/*
* Calculate proper packet lengths taking into account the
* variable size of the options field
--
2.40.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-10-24 0:21 ` [PATCH v2 1/3] net: Get pxe config file from dhcp option 209 seanedmond
@ 2023-10-24 5:54 ` Heinrich Schuchardt
2023-10-24 13:52 ` Peter Robinson
2023-11-04 1:03 ` Sean Edmond
0 siblings, 2 replies; 19+ messages in thread
From: Heinrich Schuchardt @ 2023-10-24 5:54 UTC (permalink / raw)
To: seanedmond; +Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot
On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
> From: Sean Edmond <seanedmond@microsoft.com>
>
> Allow dhcp server pass pxe config file full path by using option 209
>
> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> ---
> cmd/Kconfig | 4 ++++
> cmd/pxe.c | 10 ++++++++++
> net/bootp.c | 21 +++++++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5bc0a92d57..adbb1a6187 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
> default 0x15 if ARM
> default 0x0 if X86
>
> +config BOOTP_PXE_DHCP_OPTION
> + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
> + depends on BOOTP_PXE
Why should this be disabled by default?
Do we really want a separate config variable?
> +
> config BOOTP_VCI_STRING
> string
> depends on CMD_BOOTP
> diff --git a/cmd/pxe.c b/cmd/pxe.c
> index 704589702f..9404f44518 100644
> --- a/cmd/pxe.c
> +++ b/cmd/pxe.c
> @@ -65,6 +65,8 @@ static int pxe_dhcp_option_path(struct pxe_context *ctx, unsigned long pxefile_a
> int ret = get_pxe_file(ctx, pxelinux_configfile, pxefile_addr_r);
>
> free(pxelinux_configfile);
> + /* set to NULL to avoid double-free if DHCP is tried again */
> + pxelinux_configfile = NULL;
>
> return ret;
> }
> @@ -141,6 +143,14 @@ int pxe_get(ulong pxefile_addr_r, char **bootdirp, ulong *sizep, bool use_ipv6)
> env_get("bootfile"), use_ipv6))
> return -ENOMEM;
>
> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION) &&
> + pxelinux_configfile && !use_ipv6) {
> + if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
> + goto done;
> +
> + goto error_exit;
> + }
> +
> if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) &&
> pxelinux_configfile && use_ipv6) {
> if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
> diff --git a/net/bootp.c b/net/bootp.c
> index 7b0f45e18a..6800290963 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -26,6 +26,7 @@
> #ifdef CONFIG_BOOTP_RANDOM_DELAY
> #include "net_rand.h"
> #endif
> +#include <malloc.h>
>
> #define BOOTP_VENDOR_MAGIC 0x63825363 /* RFC1048 Magic Cookie */
>
> @@ -601,6 +602,10 @@ static int dhcp_extended(u8 *e, int message_type, struct in_addr server_ip,
> *e++ = 42;
> *cnt += 1;
> #endif
> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
> + *e++ = 209; /* PXELINUX Config File */
> + *cnt += 1;
> + }
> /* no options, so back up to avoid sending an empty request list */
> if (*cnt == 0)
> e -= 2;
> @@ -909,6 +914,22 @@ static void dhcp_process_options(uchar *popt, uchar *end)
> net_boot_file_name[size] = 0;
> }
> break;
> + case 209: /* PXELINUX Config File */
This 209 appears in multiple places. Please, define a constant, e.g.
DHCP_OPTION_CONFIG_FILE, in bootp.h and use the symbolic name. Please,
document the constant referring to RFC 5071.
> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
> + /* In case it has already been allocated when get DHCP Offer packet,
> + * free first to avoid memory leak.
> + */
> + if (pxelinux_configfile)
> + free(pxelinux_configfile);
> +
> + pxelinux_configfile = (char *)malloc((oplen + 1) * sizeof(char));
> +
> + if (pxelinux_configfile)
> + strlcpy(pxelinux_configfile, popt + 2, oplen + 1);
> + else
> + printf("Error: Failed to allocate pxelinux_configfile\n");
We do the same in dhcp6_parse_options(). Please, factor out a common
function to avoid code duplication.
Best regards
Heinrich
> + }
> + break;
> default:
> #if defined(CONFIG_BOOTP_VENDOREX)
> if (dhcp_vendorex_proc(popt))
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements
2023-10-24 0:21 ` [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements seanedmond
@ 2023-10-24 6:06 ` Heinrich Schuchardt
2023-10-24 16:42 ` Sean Edmond
2023-11-04 1:04 ` Sean Edmond
0 siblings, 2 replies; 19+ messages in thread
From: Heinrich Schuchardt @ 2023-10-24 6:06 UTC (permalink / raw)
To: seanedmond; +Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot
On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
> From: Sean Edmond <seanedmond@microsoft.com>
>
> This patch introduces 3 improvements to align with RFC 951:
> - retransmission backoff interval maximum is configurable
> - initial retranmission backoff interval is configurable
> - transaction ID is kept the same for each BOOTP/DHCPv4 request
>
> In applications where thousands of nodes are serviced by a single DHCP
> server, maximizing the retransmission backoff interval at 2 seconds (the
> current u-boot default) exerts high pressure on the DHCP server and
> network layer.
>
> RFC 951 “7.2. Client Retransmission Strategy” states that the
> retransmission backoff interval should maximize at 60 seconds. This
%s/maximize at/be limited to/
> patch allows the interval to be configurable using the environment
> variable "bootpretransmitperiodmax"
If there is an RFC defining the value, why do we need an environment
variable?
>
> The initial retranmission backoff period defaults to 250ms, which is
> also too small for these scenarios with many clients. This patch makes
> the initial retransmission interval to be configurable using the
> environment variable "bootpretransmitperiodinit".
>
> Also, on a retransmission it is not expected for the transaction ID to
> change (only the 'secs' field should be updated). Let's save the
> transaction ID and use the same transaction ID for each BOOTP/DHCPv4
> exchange.
>
> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> ---
> net/bootp.c | 56 ++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 40 insertions(+), 16 deletions(-)
>
> diff --git a/net/bootp.c b/net/bootp.c
> index 6800290963..bab17a9ceb 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -42,6 +42,17 @@
> */
> #define TIMEOUT_MS ((3 + (CONFIG_NET_RETRY_COUNT * 5)) * 1000)
>
> +/*
> + * According to rfc951 : 7.2. Client Retransmission Strategy
> + * "After the 'average' backoff reaches about 60 seconds, it should be
> + * increased no further, but still randomized."
> + *
> + * U-Boot has saturated this backoff at 2 seconds for a long time.
> + * To modify, set the environment variable "bootpretransmitperiodmax"
> + */
> +#define RETRANSMIT_PERIOD_MAX_MS 2000
This does not match RFC 951. Please, why shouldn't we use the standard
value by default?
> +#define RETRANSMIT_PERIOD_INIT_MS 250
This constant should be described too.
> +
> #ifndef CFG_DHCP_MIN_EXT_LEN /* minimal length of extension list */
> #define CFG_DHCP_MIN_EXT_LEN 64
> #endif
> @@ -53,6 +64,7 @@
> u32 bootp_ids[CFG_BOOTP_ID_CACHE_SIZE];
> unsigned int bootp_num_ids;
> int bootp_try;
> +u32 bootp_id;
> ulong bootp_start;
> ulong bootp_timeout;
> char net_nis_domain[32] = {0,}; /* Our NIS domain */
> @@ -60,6 +72,7 @@ char net_hostname[32] = {0,}; /* Our hostname */
> char net_root_path[CONFIG_BOOTP_MAX_ROOT_PATH_LEN] = {0,}; /* Our bootpath */
>
> static ulong time_taken_max;
> +static u32 retransmit_period_max_ms;
>
> #if defined(CONFIG_CMD_DHCP)
> static dhcp_state_t dhcp_state = INIT;
> @@ -414,8 +427,8 @@ static void bootp_timeout_handler(void)
> }
> } else {
> bootp_timeout *= 2;
> - if (bootp_timeout > 2000)
> - bootp_timeout = 2000;
> + if (bootp_timeout > retransmit_period_max_ms)
> + bootp_timeout = retransmit_period_max_ms;
RFC 951 requires that the backoff time is randomized.
Best regards
Heinrich
> net_set_timeout_handler(bootp_timeout, bootp_timeout_handler);
> bootp_request();
> }
> @@ -711,10 +724,14 @@ static int bootp_extended(u8 *e)
>
> void bootp_reset(void)
> {
> + char *ep; /* Environment pointer */
> +
> bootp_num_ids = 0;
> bootp_try = 0;
> bootp_start = get_timer(0);
> - bootp_timeout = 250;
> +
> + bootp_timeout = env_get_ulong("bootpretransmitperiodinit", 10, RETRANSMIT_PERIOD_INIT_MS);
> +
> }
>
> void bootp_request(void)
> @@ -726,7 +743,6 @@ void bootp_request(void)
> #ifdef CONFIG_BOOTP_RANDOM_DELAY
> ulong rand_ms;
> #endif
> - u32 bootp_id;
> struct in_addr zero_ip;
> struct in_addr bcast_ip;
> char *ep; /* Environment pointer */
> @@ -742,6 +758,9 @@ void bootp_request(void)
> else
> time_taken_max = TIMEOUT_MS;
>
> + retransmit_period_max_ms = env_get_ulong("bootpretransmitperiodmax", 10,
> + RETRANSMIT_PERIOD_MAX_MS);
> +
> #ifdef CONFIG_BOOTP_RANDOM_DELAY /* Random BOOTP delay */
> if (bootp_try == 0)
> srand_mac();
> @@ -801,18 +820,23 @@ void bootp_request(void)
> extlen = bootp_extended((u8 *)bp->bp_vend);
> #endif
>
> - /*
> - * Bootp ID is the lower 4 bytes of our ethernet address
> - * plus the current time in ms.
> - */
> - bootp_id = ((u32)net_ethaddr[2] << 24)
> - | ((u32)net_ethaddr[3] << 16)
> - | ((u32)net_ethaddr[4] << 8)
> - | (u32)net_ethaddr[5];
> - bootp_id += get_timer(0);
> - bootp_id = htonl(bootp_id);
> - bootp_add_id(bootp_id);
> - net_copy_u32(&bp->bp_id, &bootp_id);
> + /* Only generate a new transaction ID for each new BOOTP request */
> + if (bootp_try == 1) {
> + /*
> + * Bootp ID is the lower 4 bytes of our ethernet address
> + * plus the current time in ms.
> + */
> + bootp_id = ((u32)net_ethaddr[2] << 24)
> + | ((u32)net_ethaddr[3] << 16)
> + | ((u32)net_ethaddr[4] << 8)
> + | (u32)net_ethaddr[5];
> + bootp_id += get_timer(0);
> + bootp_id = htonl(bootp_id);
> + bootp_add_id(bootp_id);
> + net_copy_u32(&bp->bp_id, &bootp_id);
> + } else {
> + net_copy_u32(&bp->bp_id, &bootp_id);
> + }
>
> /*
> * Calculate proper packet lengths taking into account the
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] net: bootp: add config option BOOTP_RANDOM_XID
2023-10-24 0:21 ` [PATCH v2 3/3] net: bootp: add config option BOOTP_RANDOM_XID seanedmond
@ 2023-10-24 6:19 ` Heinrich Schuchardt
0 siblings, 0 replies; 19+ messages in thread
From: Heinrich Schuchardt @ 2023-10-24 6:19 UTC (permalink / raw)
To: seanedmond
Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot,
Jim Liu
On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
> From: Sean Edmond <seanedmond@microsoft.com>
>
> The new config option BOOTP_RANDOM_XID will randomize the transaction ID
> for each new BOOT/DHCPv4 exchange.
>
> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> ---
> cmd/Kconfig | 7 +++++++
> net/bootp.c | 31 +++++++++++++++++--------------
> 2 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index adbb1a6187..910f465d14 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1838,6 +1838,13 @@ config BOOTP_VCI_STRING
> default "U-Boot.arm" if ARM
> default "U-Boot"
>
> +config BOOTP_RANDOM_XID
> + bool "Send random transaction ID to BOOTP/DHCP server"
> + depends on CMD_BOOTP
The rand() function requires CONFIG_LIB_RAND=y or
CONFIG_EXYNOS_ACE_SHA=y or CONFIG_RNG_NPCM=y.
Best regards
Heinrich
> + help
> + Selecting this will allow for a random transaction ID to get
> + selected for each BOOTP/DHCPv4 exchange.
> +
> if CMD_DHCP6
>
> config DHCP6_PXE_CLIENTARCH
> diff --git a/net/bootp.c b/net/bootp.c
> index bab17a9ceb..f1ca2efacf 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -822,22 +822,25 @@ void bootp_request(void)
>
> /* Only generate a new transaction ID for each new BOOTP request */
> if (bootp_try == 1) {
> - /*
> - * Bootp ID is the lower 4 bytes of our ethernet address
> - * plus the current time in ms.
> - */
> - bootp_id = ((u32)net_ethaddr[2] << 24)
> - | ((u32)net_ethaddr[3] << 16)
> - | ((u32)net_ethaddr[4] << 8)
> - | (u32)net_ethaddr[5];
> - bootp_id += get_timer(0);
> - bootp_id = htonl(bootp_id);
> - bootp_add_id(bootp_id);
> - net_copy_u32(&bp->bp_id, &bootp_id);
> - } else {
> - net_copy_u32(&bp->bp_id, &bootp_id);
> + if (IS_ENABLED(CONFIG_BOOTP_RANDOM_XID)) {
> + srand(get_ticks() + rand());
> + bootp_id = rand();
> + } else {
> + /*
> + * Bootp ID is the lower 4 bytes of our ethernet address
> + * plus the current time in ms.
> + */
> + bootp_id = ((u32)net_ethaddr[2] << 24)
> + | ((u32)net_ethaddr[3] << 16)
> + | ((u32)net_ethaddr[4] << 8)
> + | (u32)net_ethaddr[5];
> + bootp_id += get_timer(0);
> + bootp_id = htonl(bootp_id);
> + }
> }
>
> + bootp_add_id(bootp_id);
> + net_copy_u32(&bp->bp_id, &bootp_id);
> /*
> * Calculate proper packet lengths taking into account the
> * variable size of the options field
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-10-24 5:54 ` Heinrich Schuchardt
@ 2023-10-24 13:52 ` Peter Robinson
2023-11-04 1:03 ` Sean Edmond
1 sibling, 0 replies; 19+ messages in thread
From: Peter Robinson @ 2023-10-24 13:52 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: seanedmond, joe.hershberger, rfried.dev, sjg, ilias.apalodimas,
u-boot
On Tue, Oct 24, 2023 at 1:30 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
> > From: Sean Edmond <seanedmond@microsoft.com>
> >
> > Allow dhcp server pass pxe config file full path by using option 209
> >
> > Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> > ---
> > cmd/Kconfig | 4 ++++
> > cmd/pxe.c | 10 ++++++++++
> > net/bootp.c | 21 +++++++++++++++++++++
> > 3 files changed, 35 insertions(+)
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 5bc0a92d57..adbb1a6187 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
> > default 0x15 if ARM
> > default 0x0 if X86
> >
> > +config BOOTP_PXE_DHCP_OPTION
> > + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
> > + depends on BOOTP_PXE
>
> Why should this be disabled by default?
>
> Do we really want a separate config variable?
I had similar thoughts, if it's a defined DHCP option I suspect most
users will want to use that for PXE, if it's not defined we'll have
the same process as we have now for the use to fall back to.
> > +
> > config BOOTP_VCI_STRING
> > string
> > depends on CMD_BOOTP
> > diff --git a/cmd/pxe.c b/cmd/pxe.c
> > index 704589702f..9404f44518 100644
> > --- a/cmd/pxe.c
> > +++ b/cmd/pxe.c
> > @@ -65,6 +65,8 @@ static int pxe_dhcp_option_path(struct pxe_context *ctx, unsigned long pxefile_a
> > int ret = get_pxe_file(ctx, pxelinux_configfile, pxefile_addr_r);
> >
> > free(pxelinux_configfile);
> > + /* set to NULL to avoid double-free if DHCP is tried again */
> > + pxelinux_configfile = NULL;
> >
> > return ret;
> > }
> > @@ -141,6 +143,14 @@ int pxe_get(ulong pxefile_addr_r, char **bootdirp, ulong *sizep, bool use_ipv6)
> > env_get("bootfile"), use_ipv6))
> > return -ENOMEM;
> >
> > + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION) &&
> > + pxelinux_configfile && !use_ipv6) {
> > + if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
> > + goto done;
> > +
> > + goto error_exit;
> > + }
> > +
> > if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) &&
> > pxelinux_configfile && use_ipv6) {
> > if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
> > diff --git a/net/bootp.c b/net/bootp.c
> > index 7b0f45e18a..6800290963 100644
> > --- a/net/bootp.c
> > +++ b/net/bootp.c
> > @@ -26,6 +26,7 @@
> > #ifdef CONFIG_BOOTP_RANDOM_DELAY
> > #include "net_rand.h"
> > #endif
> > +#include <malloc.h>
> >
> > #define BOOTP_VENDOR_MAGIC 0x63825363 /* RFC1048 Magic Cookie */
> >
> > @@ -601,6 +602,10 @@ static int dhcp_extended(u8 *e, int message_type, struct in_addr server_ip,
> > *e++ = 42;
> > *cnt += 1;
> > #endif
> > + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
> > + *e++ = 209; /* PXELINUX Config File */
> > + *cnt += 1;
> > + }
> > /* no options, so back up to avoid sending an empty request list */
> > if (*cnt == 0)
> > e -= 2;
> > @@ -909,6 +914,22 @@ static void dhcp_process_options(uchar *popt, uchar *end)
> > net_boot_file_name[size] = 0;
> > }
> > break;
> > + case 209: /* PXELINUX Config File */
>
> This 209 appears in multiple places. Please, define a constant, e.g.
> DHCP_OPTION_CONFIG_FILE, in bootp.h and use the symbolic name. Please,
> document the constant referring to RFC 5071.
>
> > + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
> > + /* In case it has already been allocated when get DHCP Offer packet,
> > + * free first to avoid memory leak.
> > + */
> > + if (pxelinux_configfile)
> > + free(pxelinux_configfile);
> > +
> > + pxelinux_configfile = (char *)malloc((oplen + 1) * sizeof(char));
> > +
> > + if (pxelinux_configfile)
> > + strlcpy(pxelinux_configfile, popt + 2, oplen + 1);
> > + else
> > + printf("Error: Failed to allocate pxelinux_configfile\n");
>
> We do the same in dhcp6_parse_options(). Please, factor out a common
> function to avoid code duplication.
>
> Best regards
>
> Heinrich
>
> > + }
> > + break;
> > default:
> > #if defined(CONFIG_BOOTP_VENDOREX)
> > if (dhcp_vendorex_proc(popt))
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements
2023-10-24 6:06 ` Heinrich Schuchardt
@ 2023-10-24 16:42 ` Sean Edmond
2023-11-04 1:04 ` Sean Edmond
1 sibling, 0 replies; 19+ messages in thread
From: Sean Edmond @ 2023-10-24 16:42 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot
On 2023-10-23 11:06 p.m., Heinrich Schuchardt wrote:
> On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
>> From: Sean Edmond <seanedmond@microsoft.com>
>>
>> This patch introduces 3 improvements to align with RFC 951:
>> - retransmission backoff interval maximum is configurable
>> - initial retranmission backoff interval is configurable
>> - transaction ID is kept the same for each BOOTP/DHCPv4 request
>>
>> In applications where thousands of nodes are serviced by a single DHCP
>> server, maximizing the retransmission backoff interval at 2 seconds (the
>> current u-boot default) exerts high pressure on the DHCP server and
>> network layer.
>>
>> RFC 951 “7.2. Client Retransmission Strategy” states that the
>> retransmission backoff interval should maximize at 60 seconds. This
>
> %s/maximize at/be limited to/
>
>> patch allows the interval to be configurable using the environment
>> variable "bootpretransmitperiodmax"
>
> If there is an RFC defining the value, why do we need an environment
> variable?
>
The intention with this patch is to provide the option to satisfy RFC,
while giving the option to preserve the existing behavior. The
retransmission timing parameters in u-boot have been the same for the
last ~10 years, so I'm guessing some have gotten used to this behavior
(even though it's not correct).
>>
>> The initial retranmission backoff period defaults to 250ms, which is
>> also too small for these scenarios with many clients. This patch makes
>> the initial retransmission interval to be configurable using the
>> environment variable "bootpretransmitperiodinit".
>>
>> Also, on a retransmission it is not expected for the transaction ID to
>> change (only the 'secs' field should be updated). Let's save the
>> transaction ID and use the same transaction ID for each BOOTP/DHCPv4
>> exchange.
>>
>> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>> ---
>> net/bootp.c | 56 ++++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/net/bootp.c b/net/bootp.c
>> index 6800290963..bab17a9ceb 100644
>> --- a/net/bootp.c
>> +++ b/net/bootp.c
>> @@ -42,6 +42,17 @@
>> */
>> #define TIMEOUT_MS ((3 + (CONFIG_NET_RETRY_COUNT * 5)) * 1000)
>>
>> +/*
>> + * According to rfc951 : 7.2. Client Retransmission Strategy
>> + * "After the 'average' backoff reaches about 60 seconds, it should be
>> + * increased no further, but still randomized."
>> + *
>> + * U-Boot has saturated this backoff at 2 seconds for a long time.
>> + * To modify, set the environment variable "bootpretransmitperiodmax"
>> + */
>> +#define RETRANSMIT_PERIOD_MAX_MS 2000
>
> This does not match RFC 951. Please, why shouldn't we use the standard
> value by default?
You're right, better to use the standard value by default.
>
>> +#define RETRANSMIT_PERIOD_INIT_MS 250
>
> This constant should be described too.
Will add a description.
>
>> +
>> #ifndef CFG_DHCP_MIN_EXT_LEN /* minimal length of extension
>> list */
>> #define CFG_DHCP_MIN_EXT_LEN 64
>> #endif
>> @@ -53,6 +64,7 @@
>> u32 bootp_ids[CFG_BOOTP_ID_CACHE_SIZE];
>> unsigned int bootp_num_ids;
>> int bootp_try;
>> +u32 bootp_id;
>> ulong bootp_start;
>> ulong bootp_timeout;
>> char net_nis_domain[32] = {0,}; /* Our NIS domain */
>> @@ -60,6 +72,7 @@ char net_hostname[32] = {0,}; /* Our hostname */
>> char net_root_path[CONFIG_BOOTP_MAX_ROOT_PATH_LEN] = {0,}; /* Our
>> bootpath */
>>
>> static ulong time_taken_max;
>> +static u32 retransmit_period_max_ms;
>>
>> #if defined(CONFIG_CMD_DHCP)
>> static dhcp_state_t dhcp_state = INIT;
>> @@ -414,8 +427,8 @@ static void bootp_timeout_handler(void)
>> }
>> } else {
>> bootp_timeout *= 2;
>> - if (bootp_timeout > 2000)
>> - bootp_timeout = 2000;
>> + if (bootp_timeout > retransmit_period_max_ms)
>> + bootp_timeout = retransmit_period_max_ms;
>
> RFC 951 requires that the backoff time is randomized.
I'll can look into adding randomization to this as well.
>
> Best regards
>
> Heinrich
>
>> net_set_timeout_handler(bootp_timeout, bootp_timeout_handler);
>> bootp_request();
>> }
>> @@ -711,10 +724,14 @@ static int bootp_extended(u8 *e)
>>
>> void bootp_reset(void)
>> {
>> + char *ep; /* Environment pointer */
>> +
>> bootp_num_ids = 0;
>> bootp_try = 0;
>> bootp_start = get_timer(0);
>> - bootp_timeout = 250;
>> +
>> + bootp_timeout = env_get_ulong("bootpretransmitperiodinit", 10,
>> RETRANSMIT_PERIOD_INIT_MS);
>> +
>> }
>>
>> void bootp_request(void)
>> @@ -726,7 +743,6 @@ void bootp_request(void)
>> #ifdef CONFIG_BOOTP_RANDOM_DELAY
>> ulong rand_ms;
>> #endif
>> - u32 bootp_id;
>> struct in_addr zero_ip;
>> struct in_addr bcast_ip;
>> char *ep; /* Environment pointer */
>> @@ -742,6 +758,9 @@ void bootp_request(void)
>> else
>> time_taken_max = TIMEOUT_MS;
>>
>> + retransmit_period_max_ms =
>> env_get_ulong("bootpretransmitperiodmax", 10,
>> + RETRANSMIT_PERIOD_MAX_MS);
>> +
>> #ifdef CONFIG_BOOTP_RANDOM_DELAY /* Random BOOTP delay */
>> if (bootp_try == 0)
>> srand_mac();
>> @@ -801,18 +820,23 @@ void bootp_request(void)
>> extlen = bootp_extended((u8 *)bp->bp_vend);
>> #endif
>>
>> - /*
>> - * Bootp ID is the lower 4 bytes of our ethernet address
>> - * plus the current time in ms.
>> - */
>> - bootp_id = ((u32)net_ethaddr[2] << 24)
>> - | ((u32)net_ethaddr[3] << 16)
>> - | ((u32)net_ethaddr[4] << 8)
>> - | (u32)net_ethaddr[5];
>> - bootp_id += get_timer(0);
>> - bootp_id = htonl(bootp_id);
>> - bootp_add_id(bootp_id);
>> - net_copy_u32(&bp->bp_id, &bootp_id);
>> + /* Only generate a new transaction ID for each new BOOTP request */
>> + if (bootp_try == 1) {
>> + /*
>> + * Bootp ID is the lower 4 bytes of our ethernet address
>> + * plus the current time in ms.
>> + */
>> + bootp_id = ((u32)net_ethaddr[2] << 24)
>> + | ((u32)net_ethaddr[3] << 16)
>> + | ((u32)net_ethaddr[4] << 8)
>> + | (u32)net_ethaddr[5];
>> + bootp_id += get_timer(0);
>> + bootp_id = htonl(bootp_id);
>> + bootp_add_id(bootp_id);
>> + net_copy_u32(&bp->bp_id, &bootp_id);
>> + } else {
>> + net_copy_u32(&bp->bp_id, &bootp_id);
>> + }
>>
>> /*
>> * Calculate proper packet lengths taking into account the
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-10-24 5:54 ` Heinrich Schuchardt
2023-10-24 13:52 ` Peter Robinson
@ 2023-11-04 1:03 ` Sean Edmond
2023-11-04 7:53 ` Heinrich Schuchardt
1 sibling, 1 reply; 19+ messages in thread
From: Sean Edmond @ 2023-11-04 1:03 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot
On 2023-10-23 10:54 p.m., Heinrich Schuchardt wrote:
> On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
>> From: Sean Edmond <seanedmond@microsoft.com>
>>
>> Allow dhcp server pass pxe config file full path by using option 209
>>
>> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>> ---
>> cmd/Kconfig | 4 ++++
>> cmd/pxe.c | 10 ++++++++++
>> net/bootp.c | 21 +++++++++++++++++++++
>> 3 files changed, 35 insertions(+)
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 5bc0a92d57..adbb1a6187 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
>> default 0x15 if ARM
>> default 0x0 if X86
>>
>> +config BOOTP_PXE_DHCP_OPTION
>> + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
>> + depends on BOOTP_PXE
>
> Why should this be disabled by default?
>
> Do we really want a separate config variable?
>
I expect most won't use this option to get the file path (they'll use
the default paths as per the PXE specification). It makes more sense
for me to keep it optional, like many of the other options?
>> +
>> config BOOTP_VCI_STRING
>> string
>> depends on CMD_BOOTP
>> diff --git a/cmd/pxe.c b/cmd/pxe.c
>> index 704589702f..9404f44518 100644
>> --- a/cmd/pxe.c
>> +++ b/cmd/pxe.c
>> @@ -65,6 +65,8 @@ static int pxe_dhcp_option_path(struct pxe_context
>> *ctx, unsigned long pxefile_a
>> int ret = get_pxe_file(ctx, pxelinux_configfile, pxefile_addr_r);
>>
>> free(pxelinux_configfile);
>> + /* set to NULL to avoid double-free if DHCP is tried again */
>> + pxelinux_configfile = NULL;
>>
>> return ret;
>> }
>> @@ -141,6 +143,14 @@ int pxe_get(ulong pxefile_addr_r, char
>> **bootdirp, ulong *sizep, bool use_ipv6)
>> env_get("bootfile"), use_ipv6))
>> return -ENOMEM;
>>
>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION) &&
>> + pxelinux_configfile && !use_ipv6) {
>> + if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
>> + goto done;
>> +
>> + goto error_exit;
>> + }
>> +
>> if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) &&
>> pxelinux_configfile && use_ipv6) {
>> if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
>> diff --git a/net/bootp.c b/net/bootp.c
>> index 7b0f45e18a..6800290963 100644
>> --- a/net/bootp.c
>> +++ b/net/bootp.c
>> @@ -26,6 +26,7 @@
>> #ifdef CONFIG_BOOTP_RANDOM_DELAY
>> #include "net_rand.h"
>> #endif
>> +#include <malloc.h>
>>
>> #define BOOTP_VENDOR_MAGIC 0x63825363 /* RFC1048 Magic Cookie */
>>
>> @@ -601,6 +602,10 @@ static int dhcp_extended(u8 *e, int
>> message_type, struct in_addr server_ip,
>> *e++ = 42;
>> *cnt += 1;
>> #endif
>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
>> + *e++ = 209; /* PXELINUX Config File */
>> + *cnt += 1;
>> + }
>> /* no options, so back up to avoid sending an empty request
>> list */
>> if (*cnt == 0)
>> e -= 2;
>> @@ -909,6 +914,22 @@ static void dhcp_process_options(uchar *popt,
>> uchar *end)
>> net_boot_file_name[size] = 0;
>> }
>> break;
>> + case 209: /* PXELINUX Config File */
>
> This 209 appears in multiple places. Please, define a constant, e.g.
> DHCP_OPTION_CONFIG_FILE, in bootp.h and use the symbolic name. Please,
> document the constant referring to RFC 5071.
fixed in v3.
>
>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
>> + /* In case it has already been allocated when get
>> DHCP Offer packet,
>> + * free first to avoid memory leak.
>> + */
>> + if (pxelinux_configfile)
>> + free(pxelinux_configfile);
>> +
>> + pxelinux_configfile = (char *)malloc((oplen + 1) *
>> sizeof(char));
>> +
>> + if (pxelinux_configfile)
>> + strlcpy(pxelinux_configfile, popt + 2, oplen + 1);
>> + else
>> + printf("Error: Failed to allocate
>> pxelinux_configfile\n");
>
> We do the same in dhcp6_parse_options(). Please, factor out a common
> function to avoid code duplication.
I would prefer to keep these seperate for several reasons:
- our DHCP server team has asked for the DHCP6 "pxe config file" parsing
to change. I attempted to add to upstream here:
https://lore.kernel.org/u-boot/20230725231329.5653-1-seanedmond@linux.microsoft.com/.
I will attempt to submitted that patch again
- PXE config file isn't standardized yet for DHCPv6 (the current
implementation was a proposal from our DHCP server team)
- LWIP will eventually superceed bootp.c in u-boot, but we'll probably
be keeping dhcpv6.c for a bit (keeping the duplicate code will make the
migration easier)
>
> Best regards
>
> Heinrich
>
>> + }
>> + break;
>> default:
>> #if defined(CONFIG_BOOTP_VENDOREX)
>> if (dhcp_vendorex_proc(popt))
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements
2023-10-24 6:06 ` Heinrich Schuchardt
2023-10-24 16:42 ` Sean Edmond
@ 2023-11-04 1:04 ` Sean Edmond
2023-11-04 7:48 ` Heinrich Schuchardt
1 sibling, 1 reply; 19+ messages in thread
From: Sean Edmond @ 2023-11-04 1:04 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot
On 2023-10-23 11:06 p.m., Heinrich Schuchardt wrote:
> On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
>> From: Sean Edmond <seanedmond@microsoft.com>
>>
>> This patch introduces 3 improvements to align with RFC 951:
>> - retransmission backoff interval maximum is configurable
>> - initial retranmission backoff interval is configurable
>> - transaction ID is kept the same for each BOOTP/DHCPv4 request
>>
>> In applications where thousands of nodes are serviced by a single DHCP
>> server, maximizing the retransmission backoff interval at 2 seconds (the
>> current u-boot default) exerts high pressure on the DHCP server and
>> network layer.
>>
>> RFC 951 “7.2. Client Retransmission Strategy” states that the
>> retransmission backoff interval should maximize at 60 seconds. This
>
> %s/maximize at/be limited to/
>
>> patch allows the interval to be configurable using the environment
>> variable "bootpretransmitperiodmax"
>
> If there is an RFC defining the value, why do we need an environment
> variable?
>
>>
>> The initial retranmission backoff period defaults to 250ms, which is
>> also too small for these scenarios with many clients. This patch makes
>> the initial retransmission interval to be configurable using the
>> environment variable "bootpretransmitperiodinit".
>>
>> Also, on a retransmission it is not expected for the transaction ID to
>> change (only the 'secs' field should be updated). Let's save the
>> transaction ID and use the same transaction ID for each BOOTP/DHCPv4
>> exchange.
>>
>> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>> ---
>> net/bootp.c | 56 ++++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/net/bootp.c b/net/bootp.c
>> index 6800290963..bab17a9ceb 100644
>> --- a/net/bootp.c
>> +++ b/net/bootp.c
>> @@ -42,6 +42,17 @@
>> */
>> #define TIMEOUT_MS ((3 + (CONFIG_NET_RETRY_COUNT * 5)) * 1000)
>>
>> +/*
>> + * According to rfc951 : 7.2. Client Retransmission Strategy
>> + * "After the 'average' backoff reaches about 60 seconds, it should be
>> + * increased no further, but still randomized."
>> + *
>> + * U-Boot has saturated this backoff at 2 seconds for a long time.
>> + * To modify, set the environment variable "bootpretransmitperiodmax"
>> + */
>> +#define RETRANSMIT_PERIOD_MAX_MS 2000
>
> This does not match RFC 951. Please, why shouldn't we use the standard
> value by default?
>
>> +#define RETRANSMIT_PERIOD_INIT_MS 250
>
> This constant should be described too.
>
>> +
>> #ifndef CFG_DHCP_MIN_EXT_LEN /* minimal length of extension
>> list */
>> #define CFG_DHCP_MIN_EXT_LEN 64
>> #endif
>> @@ -53,6 +64,7 @@
>> u32 bootp_ids[CFG_BOOTP_ID_CACHE_SIZE];
>> unsigned int bootp_num_ids;
>> int bootp_try;
>> +u32 bootp_id;
>> ulong bootp_start;
>> ulong bootp_timeout;
>> char net_nis_domain[32] = {0,}; /* Our NIS domain */
>> @@ -60,6 +72,7 @@ char net_hostname[32] = {0,}; /* Our hostname */
>> char net_root_path[CONFIG_BOOTP_MAX_ROOT_PATH_LEN] = {0,}; /* Our
>> bootpath */
>>
>> static ulong time_taken_max;
>> +static u32 retransmit_period_max_ms;
>>
>> #if defined(CONFIG_CMD_DHCP)
>> static dhcp_state_t dhcp_state = INIT;
>> @@ -414,8 +427,8 @@ static void bootp_timeout_handler(void)
>> }
>> } else {
>> bootp_timeout *= 2;
>> - if (bootp_timeout > 2000)
>> - bootp_timeout = 2000;
>> + if (bootp_timeout > retransmit_period_max_ms)
>> + bootp_timeout = retransmit_period_max_ms;
>
> RFC 951 requires that the backoff time is randomized.
>
I added randomization in v3. Note, in that update I haven't put any
guards around the use of rand()/srand() (as is the case in net_rand.h).
Perhaps it's safe to assume it will always be present for inclusion of net?
> Best regards
>
> Heinrich
>
>> net_set_timeout_handler(bootp_timeout, bootp_timeout_handler);
>> bootp_request();
>> }
>> @@ -711,10 +724,14 @@ static int bootp_extended(u8 *e)
>>
>> void bootp_reset(void)
>> {
>> + char *ep; /* Environment pointer */
>> +
>> bootp_num_ids = 0;
>> bootp_try = 0;
>> bootp_start = get_timer(0);
>> - bootp_timeout = 250;
>> +
>> + bootp_timeout = env_get_ulong("bootpretransmitperiodinit", 10,
>> RETRANSMIT_PERIOD_INIT_MS);
>> +
>> }
>>
>> void bootp_request(void)
>> @@ -726,7 +743,6 @@ void bootp_request(void)
>> #ifdef CONFIG_BOOTP_RANDOM_DELAY
>> ulong rand_ms;
>> #endif
>> - u32 bootp_id;
>> struct in_addr zero_ip;
>> struct in_addr bcast_ip;
>> char *ep; /* Environment pointer */
>> @@ -742,6 +758,9 @@ void bootp_request(void)
>> else
>> time_taken_max = TIMEOUT_MS;
>>
>> + retransmit_period_max_ms =
>> env_get_ulong("bootpretransmitperiodmax", 10,
>> + RETRANSMIT_PERIOD_MAX_MS);
>> +
>> #ifdef CONFIG_BOOTP_RANDOM_DELAY /* Random BOOTP delay */
>> if (bootp_try == 0)
>> srand_mac();
>> @@ -801,18 +820,23 @@ void bootp_request(void)
>> extlen = bootp_extended((u8 *)bp->bp_vend);
>> #endif
>>
>> - /*
>> - * Bootp ID is the lower 4 bytes of our ethernet address
>> - * plus the current time in ms.
>> - */
>> - bootp_id = ((u32)net_ethaddr[2] << 24)
>> - | ((u32)net_ethaddr[3] << 16)
>> - | ((u32)net_ethaddr[4] << 8)
>> - | (u32)net_ethaddr[5];
>> - bootp_id += get_timer(0);
>> - bootp_id = htonl(bootp_id);
>> - bootp_add_id(bootp_id);
>> - net_copy_u32(&bp->bp_id, &bootp_id);
>> + /* Only generate a new transaction ID for each new BOOTP request */
>> + if (bootp_try == 1) {
>> + /*
>> + * Bootp ID is the lower 4 bytes of our ethernet address
>> + * plus the current time in ms.
>> + */
>> + bootp_id = ((u32)net_ethaddr[2] << 24)
>> + | ((u32)net_ethaddr[3] << 16)
>> + | ((u32)net_ethaddr[4] << 8)
>> + | (u32)net_ethaddr[5];
>> + bootp_id += get_timer(0);
>> + bootp_id = htonl(bootp_id);
>> + bootp_add_id(bootp_id);
>> + net_copy_u32(&bp->bp_id, &bootp_id);
>> + } else {
>> + net_copy_u32(&bp->bp_id, &bootp_id);
>> + }
>>
>> /*
>> * Calculate proper packet lengths taking into account the
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements
2023-11-04 1:04 ` Sean Edmond
@ 2023-11-04 7:48 ` Heinrich Schuchardt
0 siblings, 0 replies; 19+ messages in thread
From: Heinrich Schuchardt @ 2023-11-04 7:48 UTC (permalink / raw)
To: Sean Edmond; +Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot
On 11/4/23 03:04, Sean Edmond wrote:
>
> On 2023-10-23 11:06 p.m., Heinrich Schuchardt wrote:
>> On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
>>> From: Sean Edmond <seanedmond@microsoft.com>
>>>
>>> This patch introduces 3 improvements to align with RFC 951:
>>> - retransmission backoff interval maximum is configurable
>>> - initial retranmission backoff interval is configurable
>>> - transaction ID is kept the same for each BOOTP/DHCPv4 request
>>>
>>> In applications where thousands of nodes are serviced by a single DHCP
>>> server, maximizing the retransmission backoff interval at 2 seconds (the
>>> current u-boot default) exerts high pressure on the DHCP server and
>>> network layer.
>>>
>>> RFC 951 “7.2. Client Retransmission Strategy” states that the
>>> retransmission backoff interval should maximize at 60 seconds. This
>>
>> %s/maximize at/be limited to/
>>
>>> patch allows the interval to be configurable using the environment
>>> variable "bootpretransmitperiodmax"
>>
>> If there is an RFC defining the value, why do we need an environment
>> variable?
>>
>>>
>>> The initial retranmission backoff period defaults to 250ms, which is
>>> also too small for these scenarios with many clients. This patch makes
>>> the initial retransmission interval to be configurable using the
>>> environment variable "bootpretransmitperiodinit".
>>>
>>> Also, on a retransmission it is not expected for the transaction ID to
>>> change (only the 'secs' field should be updated). Let's save the
>>> transaction ID and use the same transaction ID for each BOOTP/DHCPv4
>>> exchange.
>>>
>>> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>>> ---
>>> net/bootp.c | 56 ++++++++++++++++++++++++++++++++++++++---------------
>>> 1 file changed, 40 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/net/bootp.c b/net/bootp.c
>>> index 6800290963..bab17a9ceb 100644
>>> --- a/net/bootp.c
>>> +++ b/net/bootp.c
>>> @@ -42,6 +42,17 @@
>>> */
>>> #define TIMEOUT_MS ((3 + (CONFIG_NET_RETRY_COUNT * 5)) * 1000)
>>>
>>> +/*
>>> + * According to rfc951 : 7.2. Client Retransmission Strategy
>>> + * "After the 'average' backoff reaches about 60 seconds, it should be
>>> + * increased no further, but still randomized."
>>> + *
>>> + * U-Boot has saturated this backoff at 2 seconds for a long time.
>>> + * To modify, set the environment variable "bootpretransmitperiodmax"
>>> + */
>>> +#define RETRANSMIT_PERIOD_MAX_MS 2000
>>
>> This does not match RFC 951. Please, why shouldn't we use the standard
>> value by default?
>>
>>> +#define RETRANSMIT_PERIOD_INIT_MS 250
>>
>> This constant should be described too.
>>
>>> +
>>> #ifndef CFG_DHCP_MIN_EXT_LEN /* minimal length of extension
>>> list */
>>> #define CFG_DHCP_MIN_EXT_LEN 64
>>> #endif
>>> @@ -53,6 +64,7 @@
>>> u32 bootp_ids[CFG_BOOTP_ID_CACHE_SIZE];
>>> unsigned int bootp_num_ids;
>>> int bootp_try;
>>> +u32 bootp_id;
>>> ulong bootp_start;
>>> ulong bootp_timeout;
>>> char net_nis_domain[32] = {0,}; /* Our NIS domain */
>>> @@ -60,6 +72,7 @@ char net_hostname[32] = {0,}; /* Our hostname */
>>> char net_root_path[CONFIG_BOOTP_MAX_ROOT_PATH_LEN] = {0,}; /* Our
>>> bootpath */
>>>
>>> static ulong time_taken_max;
>>> +static u32 retransmit_period_max_ms;
>>>
>>> #if defined(CONFIG_CMD_DHCP)
>>> static dhcp_state_t dhcp_state = INIT;
>>> @@ -414,8 +427,8 @@ static void bootp_timeout_handler(void)
>>> }
>>> } else {
>>> bootp_timeout *= 2;
>>> - if (bootp_timeout > 2000)
>>> - bootp_timeout = 2000;
>>> + if (bootp_timeout > retransmit_period_max_ms)
>>> + bootp_timeout = retransmit_period_max_ms;
>>
>> RFC 951 requires that the backoff time is randomized.
>>
> I added randomization in v3. Note, in that update I haven't put any
> guards around the use of rand()/srand() (as is the case in net_rand.h).
> Perhaps it's safe to assume it will always be present for inclusion of net?
If you need rand(), you have to ensure that CONFIG_LIB_RAND=y, e.g. let
CONFIG_CMD_BOOTP select CONFIG_LIB_RAND.
Best regards
Heinrich
>
>> Best regards
>>
>> Heinrich
>>
>>> net_set_timeout_handler(bootp_timeout, bootp_timeout_handler);
>>> bootp_request();
>>> }
>>> @@ -711,10 +724,14 @@ static int bootp_extended(u8 *e)
>>>
>>> void bootp_reset(void)
>>> {
>>> + char *ep; /* Environment pointer */
>>> +
>>> bootp_num_ids = 0;
>>> bootp_try = 0;
>>> bootp_start = get_timer(0);
>>> - bootp_timeout = 250;
>>> +
>>> + bootp_timeout = env_get_ulong("bootpretransmitperiodinit", 10,
>>> RETRANSMIT_PERIOD_INIT_MS);
>>> +
>>> }
>>>
>>> void bootp_request(void)
>>> @@ -726,7 +743,6 @@ void bootp_request(void)
>>> #ifdef CONFIG_BOOTP_RANDOM_DELAY
>>> ulong rand_ms;
>>> #endif
>>> - u32 bootp_id;
>>> struct in_addr zero_ip;
>>> struct in_addr bcast_ip;
>>> char *ep; /* Environment pointer */
>>> @@ -742,6 +758,9 @@ void bootp_request(void)
>>> else
>>> time_taken_max = TIMEOUT_MS;
>>>
>>> + retransmit_period_max_ms =
>>> env_get_ulong("bootpretransmitperiodmax", 10,
>>> + RETRANSMIT_PERIOD_MAX_MS);
>>> +
>>> #ifdef CONFIG_BOOTP_RANDOM_DELAY /* Random BOOTP delay */
>>> if (bootp_try == 0)
>>> srand_mac();
>>> @@ -801,18 +820,23 @@ void bootp_request(void)
>>> extlen = bootp_extended((u8 *)bp->bp_vend);
>>> #endif
>>>
>>> - /*
>>> - * Bootp ID is the lower 4 bytes of our ethernet address
>>> - * plus the current time in ms.
>>> - */
>>> - bootp_id = ((u32)net_ethaddr[2] << 24)
>>> - | ((u32)net_ethaddr[3] << 16)
>>> - | ((u32)net_ethaddr[4] << 8)
>>> - | (u32)net_ethaddr[5];
>>> - bootp_id += get_timer(0);
>>> - bootp_id = htonl(bootp_id);
>>> - bootp_add_id(bootp_id);
>>> - net_copy_u32(&bp->bp_id, &bootp_id);
>>> + /* Only generate a new transaction ID for each new BOOTP request */
>>> + if (bootp_try == 1) {
>>> + /*
>>> + * Bootp ID is the lower 4 bytes of our ethernet address
>>> + * plus the current time in ms.
>>> + */
>>> + bootp_id = ((u32)net_ethaddr[2] << 24)
>>> + | ((u32)net_ethaddr[3] << 16)
>>> + | ((u32)net_ethaddr[4] << 8)
>>> + | (u32)net_ethaddr[5];
>>> + bootp_id += get_timer(0);
>>> + bootp_id = htonl(bootp_id);
>>> + bootp_add_id(bootp_id);
>>> + net_copy_u32(&bp->bp_id, &bootp_id);
>>> + } else {
>>> + net_copy_u32(&bp->bp_id, &bootp_id);
>>> + }
>>>
>>> /*
>>> * Calculate proper packet lengths taking into account the
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-11-04 1:03 ` Sean Edmond
@ 2023-11-04 7:53 ` Heinrich Schuchardt
2023-11-07 23:50 ` Sean Edmond
0 siblings, 1 reply; 19+ messages in thread
From: Heinrich Schuchardt @ 2023-11-04 7:53 UTC (permalink / raw)
To: Sean Edmond; +Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot
On 11/4/23 03:03, Sean Edmond wrote:
>
> On 2023-10-23 10:54 p.m., Heinrich Schuchardt wrote:
>> On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
>>> From: Sean Edmond <seanedmond@microsoft.com>
>>>
>>> Allow dhcp server pass pxe config file full path by using option 209
>>>
>>> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>>> ---
>>> cmd/Kconfig | 4 ++++
>>> cmd/pxe.c | 10 ++++++++++
>>> net/bootp.c | 21 +++++++++++++++++++++
>>> 3 files changed, 35 insertions(+)
>>>
>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>> index 5bc0a92d57..adbb1a6187 100644
>>> --- a/cmd/Kconfig
>>> +++ b/cmd/Kconfig
>>> @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
>>> default 0x15 if ARM
>>> default 0x0 if X86
>>>
>>> +config BOOTP_PXE_DHCP_OPTION
>>> + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
>>> + depends on BOOTP_PXE
>>
>> Why should this be disabled by default?
>>
>> Do we really want a separate config variable?
>>
> I expect most won't use this option to get the file path (they'll use
> the default paths as per the PXE specification). It makes more sense
> for me to keep it optional, like many of the other options?
RFC 5701 seems to require this option. Hence we should make it default
yes. Boards that have a build size issue can opt out.
Best regards
Heinrich
>>> +
>>> config BOOTP_VCI_STRING
>>> string
>>> depends on CMD_BOOTP
>>> diff --git a/cmd/pxe.c b/cmd/pxe.c
>>> index 704589702f..9404f44518 100644
>>> --- a/cmd/pxe.c
>>> +++ b/cmd/pxe.c
>>> @@ -65,6 +65,8 @@ static int pxe_dhcp_option_path(struct pxe_context
>>> *ctx, unsigned long pxefile_a
>>> int ret = get_pxe_file(ctx, pxelinux_configfile, pxefile_addr_r);
>>>
>>> free(pxelinux_configfile);
>>> + /* set to NULL to avoid double-free if DHCP is tried again */
>>> + pxelinux_configfile = NULL;
>>>
>>> return ret;
>>> }
>>> @@ -141,6 +143,14 @@ int pxe_get(ulong pxefile_addr_r, char
>>> **bootdirp, ulong *sizep, bool use_ipv6)
>>> env_get("bootfile"), use_ipv6))
>>> return -ENOMEM;
>>>
>>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION) &&
>>> + pxelinux_configfile && !use_ipv6) {
>>> + if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
>>> + goto done;
>>> +
>>> + goto error_exit;
>>> + }
>>> +
>>> if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) &&
>>> pxelinux_configfile && use_ipv6) {
>>> if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
>>> diff --git a/net/bootp.c b/net/bootp.c
>>> index 7b0f45e18a..6800290963 100644
>>> --- a/net/bootp.c
>>> +++ b/net/bootp.c
>>> @@ -26,6 +26,7 @@
>>> #ifdef CONFIG_BOOTP_RANDOM_DELAY
>>> #include "net_rand.h"
>>> #endif
>>> +#include <malloc.h>
>>>
>>> #define BOOTP_VENDOR_MAGIC 0x63825363 /* RFC1048 Magic Cookie */
>>>
>>> @@ -601,6 +602,10 @@ static int dhcp_extended(u8 *e, int
>>> message_type, struct in_addr server_ip,
>>> *e++ = 42;
>>> *cnt += 1;
>>> #endif
>>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
>>> + *e++ = 209; /* PXELINUX Config File */
>>> + *cnt += 1;
>>> + }
>>> /* no options, so back up to avoid sending an empty request
>>> list */
>>> if (*cnt == 0)
>>> e -= 2;
>>> @@ -909,6 +914,22 @@ static void dhcp_process_options(uchar *popt,
>>> uchar *end)
>>> net_boot_file_name[size] = 0;
>>> }
>>> break;
>>> + case 209: /* PXELINUX Config File */
>>
>> This 209 appears in multiple places. Please, define a constant, e.g.
>> DHCP_OPTION_CONFIG_FILE, in bootp.h and use the symbolic name. Please,
>> document the constant referring to RFC 5071.
> fixed in v3.
>>
>>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
>>> + /* In case it has already been allocated when get
>>> DHCP Offer packet,
>>> + * free first to avoid memory leak.
>>> + */
>>> + if (pxelinux_configfile)
>>> + free(pxelinux_configfile);
>>> +
>>> + pxelinux_configfile = (char *)malloc((oplen + 1) *
>>> sizeof(char));
>>> +
>>> + if (pxelinux_configfile)
>>> + strlcpy(pxelinux_configfile, popt + 2, oplen + 1);
>>> + else
>>> + printf("Error: Failed to allocate
>>> pxelinux_configfile\n");
>>
>> We do the same in dhcp6_parse_options(). Please, factor out a common
>> function to avoid code duplication.
> I would prefer to keep these seperate for several reasons:
> - our DHCP server team has asked for the DHCP6 "pxe config file" parsing
> to change. I attempted to add to upstream here:
> https://lore.kernel.org/u-boot/20230725231329.5653-1-seanedmond@linux.microsoft.com/. I will attempt to submitted that patch again
> - PXE config file isn't standardized yet for DHCPv6 (the current
> implementation was a proposal from our DHCP server team)
> - LWIP will eventually superceed bootp.c in u-boot, but we'll probably
> be keeping dhcpv6.c for a bit (keeping the duplicate code will make the
> migration easier)
>>
>> Best regards
>>
>> Heinrich
>>
>>> + }
>>> + break;
>>> default:
>>> #if defined(CONFIG_BOOTP_VENDOREX)
>>> if (dhcp_vendorex_proc(popt))
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-11-04 7:53 ` Heinrich Schuchardt
@ 2023-11-07 23:50 ` Sean Edmond
2023-11-08 0:23 ` Tom Rini
0 siblings, 1 reply; 19+ messages in thread
From: Sean Edmond @ 2023-11-07 23:50 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: joe.hershberger, rfried.dev, sjg, ilias.apalodimas, u-boot
On 2023-11-04 12:53 a.m., Heinrich Schuchardt wrote:
> On 11/4/23 03:03, Sean Edmond wrote:
>>
>> On 2023-10-23 10:54 p.m., Heinrich Schuchardt wrote:
>>> On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
>>>> From: Sean Edmond <seanedmond@microsoft.com>
>>>>
>>>> Allow dhcp server pass pxe config file full path by using option 209
>>>>
>>>> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>>>> ---
>>>> cmd/Kconfig | 4 ++++
>>>> cmd/pxe.c | 10 ++++++++++
>>>> net/bootp.c | 21 +++++++++++++++++++++
>>>> 3 files changed, 35 insertions(+)
>>>>
>>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>>> index 5bc0a92d57..adbb1a6187 100644
>>>> --- a/cmd/Kconfig
>>>> +++ b/cmd/Kconfig
>>>> @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
>>>> default 0x15 if ARM
>>>> default 0x0 if X86
>>>>
>>>> +config BOOTP_PXE_DHCP_OPTION
>>>> + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
>>>> + depends on BOOTP_PXE
>>>
>>> Why should this be disabled by default?
>>>
>>> Do we really want a separate config variable?
>>>
>> I expect most won't use this option to get the file path (they'll use
>> the default paths as per the PXE specification). It makes more sense
>> for me to keep it optional, like many of the other options?
>
> RFC 5701 seems to require this option. Hence we should make it default
> yes. Boards that have a build size issue can opt out.
The PXELINUX specification
(https://wiki.syslinux.org/wiki/index.php?title=PXELINUX) doesn't state
that option 209 is required. In the abense of option 209, PXELINUX will
try the following default configuration files (this example is provided
on the wiki link):
/mybootdir/pxelinux.cfg/b8945908-d6a6-41a9-611d-74a6ab80b83d
/mybootdir/pxelinux.cfg/01-88-99-aa-bb-cc-dd
/mybootdir/pxelinux.cfg/C0A8025B
/mybootdir/pxelinux.cfg/C0A8025
/mybootdir/pxelinux.cfg/C0A802
/mybootdir/pxelinux.cfg/C0A80
/mybootdir/pxelinux.cfg/C0A8
/mybootdir/pxelinux.cfg/C0A
/mybootdir/pxelinux.cfg/C0
/mybootdir/pxelinux.cfg/C
/mybootdir/pxelinux.cfg/default
If 209 is requested/provided it tries the provided "config file" before
the default paths. RFC 5071 should be seen as an extension for PXELINUX
(not a requirement). RFC 5071 only states "The Config File Option MUST
be supplied by the DHCP server if it appears on the Parameter Request List".
>
> Best regards
>
> Heinrich
>
>>>> +
>>>> config BOOTP_VCI_STRING
>>>> string
>>>> depends on CMD_BOOTP
>>>> diff --git a/cmd/pxe.c b/cmd/pxe.c
>>>> index 704589702f..9404f44518 100644
>>>> --- a/cmd/pxe.c
>>>> +++ b/cmd/pxe.c
>>>> @@ -65,6 +65,8 @@ static int pxe_dhcp_option_path(struct pxe_context
>>>> *ctx, unsigned long pxefile_a
>>>> int ret = get_pxe_file(ctx, pxelinux_configfile,
>>>> pxefile_addr_r);
>>>>
>>>> free(pxelinux_configfile);
>>>> + /* set to NULL to avoid double-free if DHCP is tried again */
>>>> + pxelinux_configfile = NULL;
>>>>
>>>> return ret;
>>>> }
>>>> @@ -141,6 +143,14 @@ int pxe_get(ulong pxefile_addr_r, char
>>>> **bootdirp, ulong *sizep, bool use_ipv6)
>>>> env_get("bootfile"), use_ipv6))
>>>> return -ENOMEM;
>>>>
>>>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION) &&
>>>> + pxelinux_configfile && !use_ipv6) {
>>>> + if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
>>>> + goto done;
>>>> +
>>>> + goto error_exit;
>>>> + }
>>>> +
>>>> if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) &&
>>>> pxelinux_configfile && use_ipv6) {
>>>> if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
>>>> diff --git a/net/bootp.c b/net/bootp.c
>>>> index 7b0f45e18a..6800290963 100644
>>>> --- a/net/bootp.c
>>>> +++ b/net/bootp.c
>>>> @@ -26,6 +26,7 @@
>>>> #ifdef CONFIG_BOOTP_RANDOM_DELAY
>>>> #include "net_rand.h"
>>>> #endif
>>>> +#include <malloc.h>
>>>>
>>>> #define BOOTP_VENDOR_MAGIC 0x63825363 /* RFC1048 Magic
>>>> Cookie */
>>>>
>>>> @@ -601,6 +602,10 @@ static int dhcp_extended(u8 *e, int
>>>> message_type, struct in_addr server_ip,
>>>> *e++ = 42;
>>>> *cnt += 1;
>>>> #endif
>>>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
>>>> + *e++ = 209; /* PXELINUX Config File */
>>>> + *cnt += 1;
>>>> + }
>>>> /* no options, so back up to avoid sending an empty request
>>>> list */
>>>> if (*cnt == 0)
>>>> e -= 2;
>>>> @@ -909,6 +914,22 @@ static void dhcp_process_options(uchar *popt,
>>>> uchar *end)
>>>> net_boot_file_name[size] = 0;
>>>> }
>>>> break;
>>>> + case 209: /* PXELINUX Config File */
>>>
>>> This 209 appears in multiple places. Please, define a constant, e.g.
>>> DHCP_OPTION_CONFIG_FILE, in bootp.h and use the symbolic name. Please,
>>> document the constant referring to RFC 5071.
>> fixed in v3.
>>>
>>>> + if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
>>>> + /* In case it has already been allocated when get
>>>> DHCP Offer packet,
>>>> + * free first to avoid memory leak.
>>>> + */
>>>> + if (pxelinux_configfile)
>>>> + free(pxelinux_configfile);
>>>> +
>>>> + pxelinux_configfile = (char *)malloc((oplen + 1) *
>>>> sizeof(char));
>>>> +
>>>> + if (pxelinux_configfile)
>>>> + strlcpy(pxelinux_configfile, popt + 2, oplen +
>>>> 1);
>>>> + else
>>>> + printf("Error: Failed to allocate
>>>> pxelinux_configfile\n");
>>>
>>> We do the same in dhcp6_parse_options(). Please, factor out a common
>>> function to avoid code duplication.
>> I would prefer to keep these seperate for several reasons:
>> - our DHCP server team has asked for the DHCP6 "pxe config file" parsing
>> to change. I attempted to add to upstream here:
>> https://lore.kernel.org/u-boot/20230725231329.5653-1-seanedmond@linux.microsoft.com/.
>> I will attempt to submitted that patch again
>> - PXE config file isn't standardized yet for DHCPv6 (the current
>> implementation was a proposal from our DHCP server team)
>> - LWIP will eventually superceed bootp.c in u-boot, but we'll probably
>> be keeping dhcpv6.c for a bit (keeping the duplicate code will make the
>> migration easier)
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> + }
>>>> + break;
>>>> default:
>>>> #if defined(CONFIG_BOOTP_VENDOREX)
>>>> if (dhcp_vendorex_proc(popt))
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-11-07 23:50 ` Sean Edmond
@ 2023-11-08 0:23 ` Tom Rini
2023-11-08 12:24 ` Peter Robinson
0 siblings, 1 reply; 19+ messages in thread
From: Tom Rini @ 2023-11-08 0:23 UTC (permalink / raw)
To: Sean Edmond
Cc: Heinrich Schuchardt, joe.hershberger, rfried.dev, sjg,
ilias.apalodimas, u-boot
[-- Attachment #1: Type: text/plain, Size: 3402 bytes --]
On Tue, Nov 07, 2023 at 03:50:06PM -0800, Sean Edmond wrote:
>
> On 2023-11-04 12:53 a.m., Heinrich Schuchardt wrote:
> > On 11/4/23 03:03, Sean Edmond wrote:
> > >
> > > On 2023-10-23 10:54 p.m., Heinrich Schuchardt wrote:
> > > > On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:
> > > > > From: Sean Edmond <seanedmond@microsoft.com>
> > > > >
> > > > > Allow dhcp server pass pxe config file full path by using option 209
> > > > >
> > > > > Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> > > > > ---
> > > > > cmd/Kconfig | 4 ++++
> > > > > cmd/pxe.c | 10 ++++++++++
> > > > > net/bootp.c | 21 +++++++++++++++++++++
> > > > > 3 files changed, 35 insertions(+)
> > > > >
> > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > index 5bc0a92d57..adbb1a6187 100644
> > > > > --- a/cmd/Kconfig
> > > > > +++ b/cmd/Kconfig
> > > > > @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
> > > > > default 0x15 if ARM
> > > > > default 0x0 if X86
> > > > >
> > > > > +config BOOTP_PXE_DHCP_OPTION
> > > > > + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
> > > > > + depends on BOOTP_PXE
> > > >
> > > > Why should this be disabled by default?
> > > >
> > > > Do we really want a separate config variable?
> > > >
> > > I expect most won't use this option to get the file path (they'll use
> > > the default paths as per the PXE specification). It makes more sense
> > > for me to keep it optional, like many of the other options?
> >
> > RFC 5701 seems to require this option. Hence we should make it default
> > yes. Boards that have a build size issue can opt out.
>
> The PXELINUX specification
> (https://wiki.syslinux.org/wiki/index.php?title=PXELINUX) doesn't state that
> option 209 is required. In the abense of option 209, PXELINUX will try the
> following default configuration files (this example is provided on the wiki
> link):
> /mybootdir/pxelinux.cfg/b8945908-d6a6-41a9-611d-74a6ab80b83d
> /mybootdir/pxelinux.cfg/01-88-99-aa-bb-cc-dd
> /mybootdir/pxelinux.cfg/C0A8025B
> /mybootdir/pxelinux.cfg/C0A8025
> /mybootdir/pxelinux.cfg/C0A802
> /mybootdir/pxelinux.cfg/C0A80
> /mybootdir/pxelinux.cfg/C0A8
> /mybootdir/pxelinux.cfg/C0A
> /mybootdir/pxelinux.cfg/C0
> /mybootdir/pxelinux.cfg/C
> /mybootdir/pxelinux.cfg/default
>
> If 209 is requested/provided it tries the provided "config file" before the
> default paths. RFC 5071 should be seen as an extension for PXELINUX (not a
> requirement). RFC 5071 only states "The Config File Option MUST be supplied
> by the DHCP server if it appears on the Parameter Request List".
We also want to be careful about overall size growth on common options,
even if someone can opt-out. Those are usually last-ditch stop-gaps and
it's better to make sure we really need functionality X/Y/Z by default,
for everyone. Especially with all of the work going on to make HTTP(s)
based network installs a viable option, how much more do we want to
change the PXE case, for everyone. I'm personally somewhat looking for
the use case to be well defined for a "this must be default". Network
provisioning is a thing, yes, but for whom/what, and in turn how broadly
do we need this to be in the vast majority of our boards (since it would
come in via BOOT*DEFAULTS or DISTRO_DEFAULTS).
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-11-08 0:23 ` Tom Rini
@ 2023-11-08 12:24 ` Peter Robinson
2023-11-09 21:04 ` Tom Rini
0 siblings, 1 reply; 19+ messages in thread
From: Peter Robinson @ 2023-11-08 12:24 UTC (permalink / raw)
To: Tom Rini
Cc: Sean Edmond, Heinrich Schuchardt, joe.hershberger, rfried.dev,
sjg, ilias.apalodimas, u-boot
> > > > > > Allow dhcp server pass pxe config file full path by using option 209
> > > > > >
> > > > > > Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> > > > > > ---
> > > > > > cmd/Kconfig | 4 ++++
> > > > > > cmd/pxe.c | 10 ++++++++++
> > > > > > net/bootp.c | 21 +++++++++++++++++++++
> > > > > > 3 files changed, 35 insertions(+)
> > > > > >
> > > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > > index 5bc0a92d57..adbb1a6187 100644
> > > > > > --- a/cmd/Kconfig
> > > > > > +++ b/cmd/Kconfig
> > > > > > @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
> > > > > > default 0x15 if ARM
> > > > > > default 0x0 if X86
> > > > > >
> > > > > > +config BOOTP_PXE_DHCP_OPTION
> > > > > > + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
> > > > > > + depends on BOOTP_PXE
> > > > >
> > > > > Why should this be disabled by default?
> > > > >
> > > > > Do we really want a separate config variable?
> > > > >
> > > > I expect most won't use this option to get the file path (they'll use
> > > > the default paths as per the PXE specification). It makes more sense
> > > > for me to keep it optional, like many of the other options?
> > >
> > > RFC 5701 seems to require this option. Hence we should make it default
> > > yes. Boards that have a build size issue can opt out.
> >
> > The PXELINUX specification
> > (https://wiki.syslinux.org/wiki/index.php?title=PXELINUX) doesn't state that
> > option 209 is required. In the abense of option 209, PXELINUX will try the
> > following default configuration files (this example is provided on the wiki
> > link):
> > /mybootdir/pxelinux.cfg/b8945908-d6a6-41a9-611d-74a6ab80b83d
> > /mybootdir/pxelinux.cfg/01-88-99-aa-bb-cc-dd
> > /mybootdir/pxelinux.cfg/C0A8025B
> > /mybootdir/pxelinux.cfg/C0A8025
> > /mybootdir/pxelinux.cfg/C0A802
> > /mybootdir/pxelinux.cfg/C0A80
> > /mybootdir/pxelinux.cfg/C0A8
> > /mybootdir/pxelinux.cfg/C0A
> > /mybootdir/pxelinux.cfg/C0
> > /mybootdir/pxelinux.cfg/C
> > /mybootdir/pxelinux.cfg/default
> >
> > If 209 is requested/provided it tries the provided "config file" before the
> > default paths. RFC 5071 should be seen as an extension for PXELINUX (not a
> > requirement). RFC 5071 only states "The Config File Option MUST be supplied
> > by the DHCP server if it appears on the Parameter Request List".
>
> We also want to be careful about overall size growth on common options,
> even if someone can opt-out. Those are usually last-ditch stop-gaps and
> it's better to make sure we really need functionality X/Y/Z by default,
> for everyone. Especially with all of the work going on to make HTTP(s)
> based network installs a viable option, how much more do we want to
> change the PXE case, for everyone. I'm personally somewhat looking for
> the use case to be well defined for a "this must be default". Network
> provisioning is a thing, yes, but for whom/what, and in turn how broadly
> do we need this to be in the vast majority of our boards (since it would
> come in via BOOT*DEFAULTS or DISTRO_DEFAULTS).
If PXE is being enabled I think it's useful in that it means the PXE
client can be largely stateless because it can all be provided by
central netowork configs as opposed to either hard wiring it into
firmware or someone having to manually set them. This is likely also
useful from the PoV less reasons to have a shell if it can be dealt
with in code.
From the PoV of defaults, with or without HTTP boot, I suspect it
probably makes sense to review whether PXE as a whole is enabled by
default. That said a number of silicon vendors are actually actively
removing PXE from their reference FW in favour of HTTP Boot due to
security and a number of other reasons (easier for firewalls,
caching/CDN, edge use cases outside of the data centre).
Peter
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-11-08 12:24 ` Peter Robinson
@ 2023-11-09 21:04 ` Tom Rini
2023-11-09 21:35 ` Heinrich Schuchardt
0 siblings, 1 reply; 19+ messages in thread
From: Tom Rini @ 2023-11-09 21:04 UTC (permalink / raw)
To: Peter Robinson
Cc: Sean Edmond, Heinrich Schuchardt, joe.hershberger, rfried.dev,
sjg, ilias.apalodimas, u-boot
[-- Attachment #1: Type: text/plain, Size: 4677 bytes --]
On Wed, Nov 08, 2023 at 12:24:24PM +0000, Peter Robinson wrote:
> > > > > > > Allow dhcp server pass pxe config file full path by using option 209
> > > > > > >
> > > > > > > Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> > > > > > > ---
> > > > > > > cmd/Kconfig | 4 ++++
> > > > > > > cmd/pxe.c | 10 ++++++++++
> > > > > > > net/bootp.c | 21 +++++++++++++++++++++
> > > > > > > 3 files changed, 35 insertions(+)
> > > > > > >
> > > > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > > > index 5bc0a92d57..adbb1a6187 100644
> > > > > > > --- a/cmd/Kconfig
> > > > > > > +++ b/cmd/Kconfig
> > > > > > > @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
> > > > > > > default 0x15 if ARM
> > > > > > > default 0x0 if X86
> > > > > > >
> > > > > > > +config BOOTP_PXE_DHCP_OPTION
> > > > > > > + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
> > > > > > > + depends on BOOTP_PXE
> > > > > >
> > > > > > Why should this be disabled by default?
> > > > > >
> > > > > > Do we really want a separate config variable?
> > > > > >
> > > > > I expect most won't use this option to get the file path (they'll use
> > > > > the default paths as per the PXE specification). It makes more sense
> > > > > for me to keep it optional, like many of the other options?
> > > >
> > > > RFC 5701 seems to require this option. Hence we should make it default
> > > > yes. Boards that have a build size issue can opt out.
> > >
> > > The PXELINUX specification
> > > (https://wiki.syslinux.org/wiki/index.php?title=PXELINUX) doesn't state that
> > > option 209 is required. In the abense of option 209, PXELINUX will try the
> > > following default configuration files (this example is provided on the wiki
> > > link):
> > > /mybootdir/pxelinux.cfg/b8945908-d6a6-41a9-611d-74a6ab80b83d
> > > /mybootdir/pxelinux.cfg/01-88-99-aa-bb-cc-dd
> > > /mybootdir/pxelinux.cfg/C0A8025B
> > > /mybootdir/pxelinux.cfg/C0A8025
> > > /mybootdir/pxelinux.cfg/C0A802
> > > /mybootdir/pxelinux.cfg/C0A80
> > > /mybootdir/pxelinux.cfg/C0A8
> > > /mybootdir/pxelinux.cfg/C0A
> > > /mybootdir/pxelinux.cfg/C0
> > > /mybootdir/pxelinux.cfg/C
> > > /mybootdir/pxelinux.cfg/default
> > >
> > > If 209 is requested/provided it tries the provided "config file" before the
> > > default paths. RFC 5071 should be seen as an extension for PXELINUX (not a
> > > requirement). RFC 5071 only states "The Config File Option MUST be supplied
> > > by the DHCP server if it appears on the Parameter Request List".
> >
> > We also want to be careful about overall size growth on common options,
> > even if someone can opt-out. Those are usually last-ditch stop-gaps and
> > it's better to make sure we really need functionality X/Y/Z by default,
> > for everyone. Especially with all of the work going on to make HTTP(s)
> > based network installs a viable option, how much more do we want to
> > change the PXE case, for everyone. I'm personally somewhat looking for
> > the use case to be well defined for a "this must be default". Network
> > provisioning is a thing, yes, but for whom/what, and in turn how broadly
> > do we need this to be in the vast majority of our boards (since it would
> > come in via BOOT*DEFAULTS or DISTRO_DEFAULTS).
>
> If PXE is being enabled I think it's useful in that it means the PXE
> client can be largely stateless because it can all be provided by
> central netowork configs as opposed to either hard wiring it into
> firmware or someone having to manually set them. This is likely also
> useful from the PoV less reasons to have a shell if it can be dealt
> with in code.
>
> From the PoV of defaults, with or without HTTP boot, I suspect it
> probably makes sense to review whether PXE as a whole is enabled by
> default. That said a number of silicon vendors are actually actively
> removing PXE from their reference FW in favour of HTTP Boot due to
> security and a number of other reasons (easier for firewalls,
> caching/CDN, edge use cases outside of the data centre).
I guess I don't understand how frequent a use case PXE boot is, on the
types of platforms U-Boot is usually found on. Clearly, some people and
usecases use it. But I also think part of why it's default enabled
within the "distro" frameworks is because initially we didn't have
"parse extlinux.conf" entirely split from "do pxeboot" as the conf file
parsing started there. I guess just something to evaluate down the road,
and perhaps make it a regular thing to evaluate defaults and provide
some notice when we might disable features.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-11-09 21:04 ` Tom Rini
@ 2023-11-09 21:35 ` Heinrich Schuchardt
2023-11-09 21:40 ` Tom Rini
0 siblings, 1 reply; 19+ messages in thread
From: Heinrich Schuchardt @ 2023-11-09 21:35 UTC (permalink / raw)
To: Tom Rini, Peter Robinson
Cc: Sean Edmond, joe.hershberger, rfried.dev, sjg, ilias.apalodimas,
u-boot
Am 9. November 2023 14:04:40 GMT-07:00 schrieb Tom Rini <trini@konsulko.com>:
>On Wed, Nov 08, 2023 at 12:24:24PM +0000, Peter Robinson wrote:
>> > > > > > > Allow dhcp server pass pxe config file full path by using option 209
>> > > > > > >
>> > > > > > > Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>> > > > > > > ---
>> > > > > > > cmd/Kconfig | 4 ++++
>> > > > > > > cmd/pxe.c | 10 ++++++++++
>> > > > > > > net/bootp.c | 21 +++++++++++++++++++++
>> > > > > > > 3 files changed, 35 insertions(+)
>> > > > > > >
>> > > > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
>> > > > > > > index 5bc0a92d57..adbb1a6187 100644
>> > > > > > > --- a/cmd/Kconfig
>> > > > > > > +++ b/cmd/Kconfig
>> > > > > > > @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
>> > > > > > > default 0x15 if ARM
>> > > > > > > default 0x0 if X86
>> > > > > > >
>> > > > > > > +config BOOTP_PXE_DHCP_OPTION
>> > > > > > > + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
>> > > > > > > + depends on BOOTP_PXE
>> > > > > >
>> > > > > > Why should this be disabled by default?
>> > > > > >
>> > > > > > Do we really want a separate config variable?
>> > > > > >
>> > > > > I expect most won't use this option to get the file path (they'll use
>> > > > > the default paths as per the PXE specification). It makes more sense
>> > > > > for me to keep it optional, like many of the other options?
>> > > >
>> > > > RFC 5701 seems to require this option. Hence we should make it default
>> > > > yes. Boards that have a build size issue can opt out.
>> > >
>> > > The PXELINUX specification
>> > > (https://wiki.syslinux.org/wiki/index.php?title=PXELINUX) doesn't state that
>> > > option 209 is required. In the abense of option 209, PXELINUX will try the
>> > > following default configuration files (this example is provided on the wiki
>> > > link):
>> > > /mybootdir/pxelinux.cfg/b8945908-d6a6-41a9-611d-74a6ab80b83d
>> > > /mybootdir/pxelinux.cfg/01-88-99-aa-bb-cc-dd
>> > > /mybootdir/pxelinux.cfg/C0A8025B
>> > > /mybootdir/pxelinux.cfg/C0A8025
>> > > /mybootdir/pxelinux.cfg/C0A802
>> > > /mybootdir/pxelinux.cfg/C0A80
>> > > /mybootdir/pxelinux.cfg/C0A8
>> > > /mybootdir/pxelinux.cfg/C0A
>> > > /mybootdir/pxelinux.cfg/C0
>> > > /mybootdir/pxelinux.cfg/C
>> > > /mybootdir/pxelinux.cfg/default
>> > >
>> > > If 209 is requested/provided it tries the provided "config file" before the
>> > > default paths. RFC 5071 should be seen as an extension for PXELINUX (not a
>> > > requirement). RFC 5071 only states "The Config File Option MUST be supplied
>> > > by the DHCP server if it appears on the Parameter Request List".
>> >
>> > We also want to be careful about overall size growth on common options,
>> > even if someone can opt-out. Those are usually last-ditch stop-gaps and
>> > it's better to make sure we really need functionality X/Y/Z by default,
>> > for everyone. Especially with all of the work going on to make HTTP(s)
>> > based network installs a viable option, how much more do we want to
>> > change the PXE case, for everyone. I'm personally somewhat looking for
>> > the use case to be well defined for a "this must be default". Network
>> > provisioning is a thing, yes, but for whom/what, and in turn how broadly
>> > do we need this to be in the vast majority of our boards (since it would
>> > come in via BOOT*DEFAULTS or DISTRO_DEFAULTS).
>>
>> If PXE is being enabled I think it's useful in that it means the PXE
>> client can be largely stateless because it can all be provided by
>> central netowork configs as opposed to either hard wiring it into
>> firmware or someone having to manually set them. This is likely also
>> useful from the PoV less reasons to have a shell if it can be dealt
>> with in code.
>>
>> From the PoV of defaults, with or without HTTP boot, I suspect it
>> probably makes sense to review whether PXE as a whole is enabled by
>> default. That said a number of silicon vendors are actually actively
>> removing PXE from their reference FW in favour of HTTP Boot due to
>> security and a number of other reasons (easier for firewalls,
>> caching/CDN, edge use cases outside of the data centre).
>
>I guess I don't understand how frequent a use case PXE boot is, on the
>types of platforms U-Boot is usually found on. Clearly, some people and
>usecases use it. But I also think part of why it's default enabled
>within the "distro" frameworks is because initially we didn't have
>"parse extlinux.conf" entirely split from "do pxeboot" as the conf file
>parsing started there. I guess just something to evaluate down the road,
>and perhaps make it a regular thing to evaluate defaults and provide
>some notice when we might disable features.
>
For security reason it is preferable to disable PXE boot if it is not really needed. So default no would make sense.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209
2023-11-09 21:35 ` Heinrich Schuchardt
@ 2023-11-09 21:40 ` Tom Rini
0 siblings, 0 replies; 19+ messages in thread
From: Tom Rini @ 2023-11-09 21:40 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Peter Robinson, Sean Edmond, joe.hershberger, rfried.dev, sjg,
ilias.apalodimas, u-boot
[-- Attachment #1: Type: text/plain, Size: 5590 bytes --]
On Thu, Nov 09, 2023 at 02:35:40PM -0700, Heinrich Schuchardt wrote:
>
>
> Am 9. November 2023 14:04:40 GMT-07:00 schrieb Tom Rini <trini@konsulko.com>:
> >On Wed, Nov 08, 2023 at 12:24:24PM +0000, Peter Robinson wrote:
> >> > > > > > > Allow dhcp server pass pxe config file full path by using option 209
> >> > > > > > >
> >> > > > > > > Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> >> > > > > > > ---
> >> > > > > > > cmd/Kconfig | 4 ++++
> >> > > > > > > cmd/pxe.c | 10 ++++++++++
> >> > > > > > > net/bootp.c | 21 +++++++++++++++++++++
> >> > > > > > > 3 files changed, 35 insertions(+)
> >> > > > > > >
> >> > > > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> >> > > > > > > index 5bc0a92d57..adbb1a6187 100644
> >> > > > > > > --- a/cmd/Kconfig
> >> > > > > > > +++ b/cmd/Kconfig
> >> > > > > > > @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
> >> > > > > > > default 0x15 if ARM
> >> > > > > > > default 0x0 if X86
> >> > > > > > >
> >> > > > > > > +config BOOTP_PXE_DHCP_OPTION
> >> > > > > > > + bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
> >> > > > > > > + depends on BOOTP_PXE
> >> > > > > >
> >> > > > > > Why should this be disabled by default?
> >> > > > > >
> >> > > > > > Do we really want a separate config variable?
> >> > > > > >
> >> > > > > I expect most won't use this option to get the file path (they'll use
> >> > > > > the default paths as per the PXE specification). It makes more sense
> >> > > > > for me to keep it optional, like many of the other options?
> >> > > >
> >> > > > RFC 5701 seems to require this option. Hence we should make it default
> >> > > > yes. Boards that have a build size issue can opt out.
> >> > >
> >> > > The PXELINUX specification
> >> > > (https://wiki.syslinux.org/wiki/index.php?title=PXELINUX) doesn't state that
> >> > > option 209 is required. In the abense of option 209, PXELINUX will try the
> >> > > following default configuration files (this example is provided on the wiki
> >> > > link):
> >> > > /mybootdir/pxelinux.cfg/b8945908-d6a6-41a9-611d-74a6ab80b83d
> >> > > /mybootdir/pxelinux.cfg/01-88-99-aa-bb-cc-dd
> >> > > /mybootdir/pxelinux.cfg/C0A8025B
> >> > > /mybootdir/pxelinux.cfg/C0A8025
> >> > > /mybootdir/pxelinux.cfg/C0A802
> >> > > /mybootdir/pxelinux.cfg/C0A80
> >> > > /mybootdir/pxelinux.cfg/C0A8
> >> > > /mybootdir/pxelinux.cfg/C0A
> >> > > /mybootdir/pxelinux.cfg/C0
> >> > > /mybootdir/pxelinux.cfg/C
> >> > > /mybootdir/pxelinux.cfg/default
> >> > >
> >> > > If 209 is requested/provided it tries the provided "config file" before the
> >> > > default paths. RFC 5071 should be seen as an extension for PXELINUX (not a
> >> > > requirement). RFC 5071 only states "The Config File Option MUST be supplied
> >> > > by the DHCP server if it appears on the Parameter Request List".
> >> >
> >> > We also want to be careful about overall size growth on common options,
> >> > even if someone can opt-out. Those are usually last-ditch stop-gaps and
> >> > it's better to make sure we really need functionality X/Y/Z by default,
> >> > for everyone. Especially with all of the work going on to make HTTP(s)
> >> > based network installs a viable option, how much more do we want to
> >> > change the PXE case, for everyone. I'm personally somewhat looking for
> >> > the use case to be well defined for a "this must be default". Network
> >> > provisioning is a thing, yes, but for whom/what, and in turn how broadly
> >> > do we need this to be in the vast majority of our boards (since it would
> >> > come in via BOOT*DEFAULTS or DISTRO_DEFAULTS).
> >>
> >> If PXE is being enabled I think it's useful in that it means the PXE
> >> client can be largely stateless because it can all be provided by
> >> central netowork configs as opposed to either hard wiring it into
> >> firmware or someone having to manually set them. This is likely also
> >> useful from the PoV less reasons to have a shell if it can be dealt
> >> with in code.
> >>
> >> From the PoV of defaults, with or without HTTP boot, I suspect it
> >> probably makes sense to review whether PXE as a whole is enabled by
> >> default. That said a number of silicon vendors are actually actively
> >> removing PXE from their reference FW in favour of HTTP Boot due to
> >> security and a number of other reasons (easier for firewalls,
> >> caching/CDN, edge use cases outside of the data centre).
> >
> >I guess I don't understand how frequent a use case PXE boot is, on the
> >types of platforms U-Boot is usually found on. Clearly, some people and
> >usecases use it. But I also think part of why it's default enabled
> >within the "distro" frameworks is because initially we didn't have
> >"parse extlinux.conf" entirely split from "do pxeboot" as the conf file
> >parsing started there. I guess just something to evaluate down the road,
> >and perhaps make it a regular thing to evaluate defaults and provide
> >some notice when we might disable features.
> >
>
> For security reason it is preferable to disable PXE boot if it is not really needed. So default no would make sense.
We can evaluate things and make sure everyone knows when changes are
coming, for the future. We've been enabling it by default for years and
"for security" isn't a great reason. It's one of the last targets tried
and I would argue if there's malicious actors on your network serving up
bootable payloads to random devices, there's bigger problems afoot.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-11-09 21:40 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-24 0:21 [PATCH v2 0/3] BOOTP/DHCPv4 enhancements seanedmond
2023-10-24 0:21 ` [PATCH v2 1/3] net: Get pxe config file from dhcp option 209 seanedmond
2023-10-24 5:54 ` Heinrich Schuchardt
2023-10-24 13:52 ` Peter Robinson
2023-11-04 1:03 ` Sean Edmond
2023-11-04 7:53 ` Heinrich Schuchardt
2023-11-07 23:50 ` Sean Edmond
2023-11-08 0:23 ` Tom Rini
2023-11-08 12:24 ` Peter Robinson
2023-11-09 21:04 ` Tom Rini
2023-11-09 21:35 ` Heinrich Schuchardt
2023-11-09 21:40 ` Tom Rini
2023-10-24 0:21 ` [PATCH v2 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements seanedmond
2023-10-24 6:06 ` Heinrich Schuchardt
2023-10-24 16:42 ` Sean Edmond
2023-11-04 1:04 ` Sean Edmond
2023-11-04 7:48 ` Heinrich Schuchardt
2023-10-24 0:21 ` [PATCH v2 3/3] net: bootp: add config option BOOTP_RANDOM_XID seanedmond
2023-10-24 6:19 ` Heinrich Schuchardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox