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

[Heinrich Schuchardt]

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 at linux.microsoft.com wrote:
>>> From: Sean Edmond <seanedmond at microsoft.com>
>>>
>>> Allow dhcp server pass pxe config file full path by using option 209
>>>
>>> Signed-off-by: Sean Edmond <seanedmond at 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))