Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209

[Sean Edmond <seanedmond@linux.microsoft.com>]

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))