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

[Peter Robinson]

On Tue, Oct 24, 2023 at 1:30 PM Heinrich Schuchardt <xypron.glpk at gmx.de> 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 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))
>