Re: [PATCH v2 5/6] dhcp: Add option overload

[Anshu Kumari <anskuma@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7051 bytes --]

On Wed, May 27, 2026 at 9:46 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> On Tue, May 26, 2026 at 06:01:12PM +0530, Anshu Kumari wrote:
> > A user can enter lots of options in command-line which may not fit in
> > existing buffer, So when the options field is full, overflow remaining
> > DHCP options into the file and sname fields per RFC 2132 option 52.
> >
> > Also, when the file field is not used for overload, copy the boot
> > file URL there directly for legacy PXE clients.
> >
> > Link: https://bugs.passt.top/show_bug.cgi?id=192
> > Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> > ---
> > v2:
> >   - Added #define DHCP_OVERLOAD_FILE and #define DHCP_OVERLOAD_SNAME
> constants
> >   - Added comment documenting space reservation: /* Reserve 3 bytes for
> option 52 */
> >   - Fixed DNS search length: sizeof(m->o) only, not combined with
> file+sname
> >   - Removed dhcp_boot references — reply.file copy now reads from
> opts[67]
> >   - Used DHCP_OVERLOAD_FILE constant in reply.file guard
> > ---
> >  dhcp.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 75 insertions(+), 9 deletions(-)
> >
> > diff --git a/dhcp.c b/dhcp.c
> > index e126063..a49a05a 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -306,14 +306,59 @@ static bool fill_one(uint8_t *buf, size_t size,
> int o, int *offset)
> >       return false;
> >  }
> >
> > +#define DHCP_OVERLOAD_FILE   1
> > +#define DHCP_OVERLOAD_SNAME  2
>
> IIUC, these values are from the RFC - might be worth referencing the
> relevant section in a comment to make it clear they can't be changed
> arbitrarily.
>
> > +
> > +/**
> > + * fill_overflow() - Fill remaining options into file and sname fields
> > + * @m:               Message whose file/sname fields may be used for
> overflow
> > + *
> > + * Return: option 52 overload value: 0 if no overflow,
> > + *         DHCP_OVERLOAD_FILE for file, DHCP_OVERLOAD_SNAME for sname,
> > + *         or both OR'd together
> > + */
> > +static int fill_overflow(struct msg *m)
> > +{
> > +     int file_off = 0, sname_off = 0, overload = 0;
> > +     int o;
> > +
> > +     for (o = 0; o < 255; o++) {
>
> You could use ARRAY_size(opts) instead of raw 255, yes?
>
> > +             if (opts[o].slen == -1 || opts[o].sent)
> > +                     continue;
> > +             fill_one(m->file, sizeof(m->file) - 1, o, &file_off);
> > +     }
> > +
> > +     for (o = 0; o < 255; o++) {
> > +             if (opts[o].slen == -1 || opts[o].sent)
> > +                     continue;
> > +             if (fill_one(m->sname, sizeof(m->sname) - 1, o,
> &sname_off))
> > +                     debug("DHCP: skipping option %i (overload full)",
> o);
> > +     }
> > +
> > +     if (file_off) {
> > +             m->file[file_off] = 255;
> > +             overload |= DHCP_OVERLOAD_FILE;
> > +     }
> > +
> > +     if (sname_off) {
> > +             m->sname[sname_off] = 255;
> > +             overload |= DHCP_OVERLOAD_SNAME;
> > +     }
> > +
> > +     return overload;
> > +}
> > +
> >  /**
> > - * fill() - Fill options in message
> > + * fill() - Fill options in message, with overload into file/sname if
> needed
> >   * @m:               Message to fill
> > + * @overload:        Set to option 52 value (0 if none, 1/2/3 per RFC
> 2132)
> >   *
> >   * Return: current size of options field
> >   */
> > -static int fill(struct msg *m)
> > +static int fill(struct msg *m, int *overload)
> >  {
> > +     /* Reserve 3 bytes for option 52 (overload) if needed */
> > +     size_t size = OPT_MAX - 3;
> >       int i, o, offset = 0;
> >
> >       for (o = 0; o < 255; o++)
> > @@ -324,20 +369,25 @@ static int fill(struct msg *m)
> >        * Put it there explicitly, unless requested via option 55.
> >        */
> >       if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
> > -             if (fill_one(m->o, OPT_MAX, 53, &offset))
> > -                      debug("DHCP: skipping option 53");
> > +             fill_one(m->o, size, 53, &offset);
> >
> >       for (i = 0; i < opts[55].clen; i++) {
> >               o = opts[55].c[i];
> >               if (opts[o].slen != -1)
> > -                     if (fill_one(m->o, OPT_MAX, o, &offset))
> > -                             debug("DHCP: skipping option %i", o);
> > +                     fill_one(m->o, size, o, &offset);
> >       }
> >
> >       for (o = 0; o < 255; o++) {
> >               if (opts[o].slen != -1 && !opts[o].sent)
> > -                     if (fill_one(m->o, OPT_MAX, o, &offset))
> > -                             debug("DHCP: skipping option %i", o);
> > +                     fill_one(m->o, size, o, &offset);
> > +     }
> > +
> > +     *overload = fill_overflow(m);
> > +
> > +     if (*overload) {
> > +             m->o[offset++] = 52;
> > +             m->o[offset++] = 1;
> > +             m->o[offset++] = *overload;
> >       }
> >
> >       m->o[offset++] = 255;
> > @@ -462,6 +512,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
> >       struct msg const *m;
> >       struct msg reply;
> >       unsigned int i;
> > +     int overload;
> >
> >       eh = IOV_REMOVE_HEADER(data, eh_storage);
> >       iph = IOV_PEEK_HEADER(data, iph_storage);
> > @@ -613,7 +664,22 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
> >       if (!c->no_dhcp_dns_search)
> >               opt_set_dns_search(c, sizeof(m->o));
> >
> > -     dlen = offsetof(struct msg, o) + fill(&reply);
> > +     for (i = 0; i < (unsigned int)c->custom_opts_count; i++) {
> > +             uint8_t code = c->custom_opts[i].code;
> > +
> > +             opts[code].slen = c->custom_opts[i].len;
> > +             memcpy(opts[code].s, c->custom_opts[i].val,
> > +                    c->custom_opts[i].len);
> > +     }
> > +
> > +     dlen = offsetof(struct msg, o) + fill(&reply, &overload);
> > +
> > +     /* Copy boot file name into the file field for legacy PXE clients,
> > +      * unless the file field is already used for option overload.
>
> Given we do this, would it make more sense to use the slen field in
> preference to the file field for overloads?  The logic above appears
> to do it the other way around,
>

are you referring to the sname field here? I am not sure how to use the
slen field instead of the file field.

>
> > +      */
> > +     if (!(overload & DHCP_OVERLOAD_FILE) &&
> > +         opts[67].slen > 0 && (size_t)opts[67].slen <
> sizeof(reply.file))
> > +             memcpy(&reply.file, opts[67].s, opts[67].slen + 1);
> >
> >       if (m->flags & FLAG_BROADCAST)
> >               dst = in4addr_broadcast;
> > --
> > 2.54.0
> >
>
> --
> David Gibson (he or they)       | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
>                                 | around.
> http://www.ozlabs.org/~dgibson
>

[-- Attachment #2: Type: text/html, Size: 9405 bytes --]