Re: [PATCH v4 2/4] dhcp: Add option overload

[Anshu Kumari <anskuma@redhat.com>]

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

On Fri, Jun 19, 2026 at 9:24 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> On Wed, Jun 17, 2026 at 06:52:36PM +0530, Anshu Kumari wrote:
> > When the options field is full, overflow remaining DHCP options into
> > the sname and file fields per RFC 2132 option 52.
> >
> > Per RFC 2132, Section 9.5, the boot file name is always placed in the
> > 'file' header field.  When a boot file is set, the file field is
> > reserved from overload and overflow uses only the sname field.
> >
> > Link: https://bugs.passt.top/show_bug.cgi?id=192
> > Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> > ---
> > v4:
> >   - Converted overload #defines to enum dhcp_overload.
> >   - Fixed missing whitespace in comment before */.
> >   - Boot file name always placed in 'file' header field per RFC 2132,
> >     Section 9.5; file field reserved from overload when bootfile is
> >     set; option 67 suppressed from options area.
> >
> > v3:
> >   - Added RFC 2132 Section 9.3 reference comment on overload
> >     constants.
> >   - Use ARRAY_SIZE(opts) instead of raw 255 in fill_overflow().
> >   - Swapped overflow order: try sname (64 bytes) first, then file
> >     (128 bytes) — better packing and keeps file field available for
> >     boot file name.
> >   - Removed '&' from &reply.file.
> >   - Removed '+1' from memcpy — reply.file already zeroed.
> >   - opt_set_dns_search() max_len: OPT_MAX - 3 instead of
> >     sizeof(m->o).
> >
> > 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 | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 100 insertions(+), 8 deletions(-)
> >
> > diff --git a/dhcp.c b/dhcp.c
> > index 8e3ffd6..78790d8 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -160,14 +160,79 @@ static bool fill_one(uint8_t *buf, size_t size,
> int o, int *offset)
> >       return false;
> >  }
> >
> > +/* RFC 2132, Section 9.3 - Option Overload */
> > +enum dhcp_overload {
> > +     DHCP_OVERLOAD_NONE  = 0,
> > +     DHCP_OVERLOAD_FILE  = 1,
> > +     DHCP_OVERLOAD_SNAME = 2,
> > +};
> > +
> >  /**
> > - * fill() - Fill options in message
> > - * @m:               Message to fill
> > + * fill_overflow() - Fill remaining options into sname and file fields
> > + * @m:                       Msg whose sname/file fields may be used
> for overflow
> > + * @has_bootfile:    If true, reserve the file field for the boot file
> name
> > + *
> > + * Try the smaller sname field first: small options go there, leaving
> > + * the larger file field available for big options. When @has_bootfile
> > + * is set, the file field is reserved for the boot file name per
> > + * RFC 2132, Section 9.5 and is not used for option overflow.
> > + *
> > + * Return: option 52 overload value
> > + */
> > +static enum dhcp_overload fill_overflow(struct msg *m, bool
> has_bootfile)
> > +{
> > +     enum dhcp_overload overload = DHCP_OVERLOAD_NONE;
> > +     int sname_off = 0, file_off = 0;
> > +     int o;
> > +
> > +     for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
> > +             if (opts[o].slen == -1 || opts[o].sent)
> > +                     continue;
> > +             fill_one(m->sname, sizeof(m->sname) - 1, o, &sname_off);
>
> Ah, I think I see now.  The earlier debug() messages were removed in
> favour of the ones later, because we don't want spurious messages if
> the option doesn't fit in the main area but does fit in the overload.
>
> That's a reasonable change, but needs to be better explained.  It also
> all belongs in this patch, so the bits that leaked into the previous
> patch should be here instead.
>
> > +     }
> > +
> > +     if (!has_bootfile) {
> > +             for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
> > +                     if (opts[o].slen == -1 || opts[o].sent)
> > +                             continue;
> > +                     if (fill_one(m->file, sizeof(m->file) - 1, o,
> > +                                  &file_off))
> > +                             debug("DHCP: skipping option %i"
> > +                                   " (overload full)", o);
> > +             }
> > +     } else {
>
> Rather that checking for fill_one() failures only on the last pass,
> then having this fallback case, you can have a single unconditional
> pass at the end looking for skipped options.  Then fill_one()s
> signature can be changed to void to match.
>
> > +             for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
> > +                     if (opts[o].slen == -1 || opts[o].sent)
> > +                             continue;
> > +                     debug("DHCP: skipping option %i (overload full)",
> o);
> > +             }
> > +     }
> > +
> > +     if (sname_off) {
> > +             m->sname[sname_off] = 255;
> > +             overload |= DHCP_OVERLOAD_SNAME;
> > +     }
> > +
> > +     if (file_off) {
> > +             m->file[file_off] = 255;
> > +             overload |= DHCP_OVERLOAD_FILE;
> > +     }
> > +
> > +     return overload;
> > +}
> > +
> > +/**
> > + * 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)
> > + * @has_bootfile:    Reserve file field for boot file name
> >   *
> >   * Return: current size of options field
> >   */
> > -static int fill(struct msg *m)
> > +static int fill(struct msg *m, enum dhcp_overload *overload, bool
> has_bootfile)
> >  {
> > +     /* 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++)
> > @@ -178,17 +243,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))
> > -             fill_one(m->o, OPT_MAX, 53, &offset);
> > +             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)
> > -                     fill_one(m->o, OPT_MAX, o, &offset);
> > +                     fill_one(m->o, size, o, &offset);
> >       }
> >
> >       for (o = 0; o < 255; o++) {
> >               if (opts[o].slen != -1 && !opts[o].sent)
> > -                     fill_one(m->o, OPT_MAX, o, &offset);
> > +                     fill_one(m->o, size, o, &offset);
> > +     }
> > +
> > +     *overload = fill_overflow(m, has_bootfile);
>
> Is there a particular reason to put fill_overflow() in its own
> function, rather than just inline here?
>

There is no particular reason. I just thought it would be better to
have a separate function for option overload (may be for better clarity).


> > +
> > +     if (*overload) {
> > +             m->o[offset++] = 52;
> > +             m->o[offset++] = 1;
> > +             m->o[offset++] = *overload;
> >       }
> >
> >       m->o[offset++] = 255;
> > @@ -301,6 +374,7 @@ static void opt_set_dns_search(const struct ctx *c,
> size_t max_len)
> >  int dhcp(const struct ctx *c, struct iov_tail *data)
> >  {
> >       char macstr[ETH_ADDRSTRLEN];
> > +     enum dhcp_overload overload;
> >       size_t mlen, dlen, opt_len;
> >       struct in_addr mask, dst;
> >       struct ethhdr eh_storage;
> > @@ -309,9 +383,12 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
> >       const struct ethhdr *eh;
> >       const struct iphdr *iph;
> >       const struct udphdr *uh;
> > +     uint8_t bootfile[128];
> >       struct msg m_storage;
> >       struct msg const *m;
> > +     bool has_bootfile;
> >       struct msg reply;
> > +     int bootfile_len;
> >       unsigned int i;
> >
> >       eh = IOV_REMOVE_HEADER(data, eh_storage);
> > @@ -462,9 +539,24 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
> >       }
> >
> >       if (!c->no_dhcp_dns_search)
> > -             opt_set_dns_search(c, sizeof(m->o));
> > +             opt_set_dns_search(c, OPT_MAX - 3);
> > +
> > +     /* RFC 2132, Section 9.5: put boot file name in the 'file' header
> > +      * field.  Suppress option 67 from the options area and reserve
> > +      * the file field from overload.
> > +      */
> > +     has_bootfile = opts[67].slen > 0 &&
> > +                    (size_t)opts[67].slen < sizeof(reply.file);
> > +     if (has_bootfile) {
> > +             memcpy(bootfile, opts[67].s, opts[67].slen);
> > +             bootfile_len = opts[67].slen;
> > +             opts[67].slen = -1;
> > +     }
> > +
> > +     dlen = offsetof(struct msg, o) + fill(&reply, &overload,
> has_bootfile);
> >
> > -     dlen = offsetof(struct msg, o) + fill(&reply);
> > +     if (has_bootfile)
> > +             memcpy(reply.file, bootfile, bootfile_len);
> >
> >       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: 12625 bytes --]