Re: [PATCH v5 6/7] dhcp: Add RFC 3396 option splitting for concatenation-requiring options

[Anshu Kumari <anskuma@redhat.com>]

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

On Mon, Jul 20, 2026 at 9:54 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> On Fri, Jul 17, 2026 at 11:26:43PM +0530, Anshu Kumari wrote:
> > Implement option splitting per RFC 3396 for options that may exceed
> > 255 bytes.  A new DHCP_OPT_STR_CONCAT type marks concatenation-
> > requiring options (currently option 81, Client FQDN per RFC 4702).
> >
> > The opts[].s buffer is resized from 255 to 496 bytes to hold the
> > maximum data that can be split across the options field, file field,
> > and sname field.
> >
> > When a concatenation-requiring option does not fit as a single option
> > in any field, fill() splits it across fields in RFC 3396 order:
> > options field first, then file, then sname.
> >
> > Link: https://bugs.passt.top/show_bug.cgi?id=192
> > Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> > ---
> > v5:
> >   - New patch: implement option splitting per RFC 3396 for options
> exceeding 255 bytes
> >   - Add DHCP_OPT_STR_CONCAT type, is_concat_opt(), fill_split() helpers
> >   - Resize opts[].s from 255 to OPT_CONCAT_MAX (496) bytes
> >   - Add /* fallthrough */ between DHCP_OPT_STR and DHCP_OPT_STR_CONCAT
> case
> >
> > ---
> >  dhcp.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 106 insertions(+), 3 deletions(-)
> >
> > diff --git a/dhcp.c b/dhcp.c
> > index cc910ee..6bebb5f 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -34,6 +34,11 @@
> >  #include "log.h"
> >  #include "dhcp.h"
> >
> > +/* RFC 3396: maximum option data that can be split across options field,
> > + * file field, and sname field (minus code+length overhead per portion).
> > + */
> > +#define OPT_CONCAT_MAX       496
> > +
> >  /**
> >   * enum opt_state - DHCP option state
> >   * @OPT_UNSET:               Option not configured
> > @@ -58,7 +63,7 @@ enum opt_state {
> >  struct opt {
> >       int sent;
> >       int slen;
> > -     uint8_t s[255];
> > +     uint8_t s[OPT_CONCAT_MAX];
> >       int clen;
> >       uint8_t c[255];
> >       enum opt_state state;
> > @@ -159,6 +164,7 @@ struct msg {
> >   * @DHCP_OPT_UINT16: Unsigned 16-bit integer
> >   * @DHCP_OPT_UINT32: Unsigned 32-bit integer
> >   * @DHCP_OPT_INT32:  Signed 32-bit integer
> > + * @DHCP_OPT_STR_CONCAT:Concatenation-requiring string (RFC 3396)
>
> It's not entirely clear to me that encoding this in the opt_type enum
> makes sense.  Generally the dhcp_opt_type is saying how the option is
> encoded as a string for the user.  This is saying how it's encoded
> within the DHCP packet itself, which seems qualitatively different
> information.
>
> Since we don't limit on the string length that user can set via
--dhcp-opt, DHCP_OPT_STR rejects
anything over 255 bytes but with STR_CONCAT longer input can be parsed.

is_concat_opt() checks dhcp_opt_types[] for STR_CONCAT to decide which
options need splitting in fill().
I think these two concerns are coupled: an option that accepts long strings
from the CLI
is exactly the same option that needs splitting on the wire.

> >   */
> >  enum dhcp_opt_type {
> >       DHCP_OPT_NONE,
> > @@ -169,6 +175,7 @@ enum dhcp_opt_type {
> >       DHCP_OPT_UINT16,
> >       DHCP_OPT_UINT32,
> >       DHCP_OPT_INT32,
> > +     DHCP_OPT_STR_CONCAT,
> >  };
> >
> >  /**
> > @@ -319,6 +326,10 @@ static int dhcp_opt_parse(uint8_t code, const char
> *str,
> >
> >               return width;
> >       case DHCP_OPT_STR:
> > +             if (strlen(str) > 255)
> > +                     return -1;
> > +             /* fallthrough */
> > +     case DHCP_OPT_STR_CONCAT:
> >               slen = strlen(str);
> >
> >               if (slen >= buf_len)
> > @@ -465,6 +476,53 @@ enum dhcp_overload {
> >       DHCP_OVERLOAD_SNAME,
> >  };
> >
> > +/**
> > + * is_concat_opt() - Check if option requires RFC 3396 concatenation
> support
> > + * @o:               Option number
> > + *
> > + * Return: true if option is a concatenation-requiring type
> > + */
> > +static bool is_concat_opt(int o)
> > +{
> > +     if ((size_t)o >= ARRAY_SIZE(dhcp_opt_types))
> > +             return false;
>
> IIUC, passing an out of bounds option code here would already be a bug
> in our code, so an assert() might make more sense.
>
> > +     return dhcp_opt_types[o] == DHCP_OPT_STR_CONCAT;
> > +}
> > +
> > +/**
> > + * fill_split() - Write a split portion of an option into a buffer
> > + * @buf:     Buffer to write into
> > + * @size:    Usable size of @buf
> > + * @o:               Option number (code)
> > + * @offset:  Current offset within @buf, updated on write
> > + * @data:    Pointer to remaining option data to write
> > + * @remaining:       Bytes of option data still to write
> > + *
> > + * Return: number of data bytes written (excluding code+length header)
> > + */
> > +static size_t fill_split(uint8_t *buf, size_t size, int o, int *offset,
> > +                      const uint8_t *data, size_t remaining)
> > +{
> > +     size_t avail, chunk;
> > +
> > +     if (*offset + 2 >= (int)size)
> > +             return 0;
> > +
> > +     avail = size - *offset - 2;
> > +     chunk = remaining < avail ? remaining : avail;
>
> You can use the existing MIN() macro here.
>
> > +     if (!chunk)
> > +             return 0;
> > +
> > +     buf[*offset] = o;
> > +     buf[*offset + 1] = chunk;
> > +     *offset += 2;
> > +
> > +     memcpy(buf + *offset, data, chunk);
> > +     *offset += chunk;
> > +
> > +     return chunk;
> > +}
> > +
> >  /**
> >   * fill() - Fill options in message, with overload into file/sname if
> needed
> >   * @m:                       Message to fill
> > @@ -513,14 +571,59 @@ static int fill(struct msg *m, enum dhcp_overload
> *overload, bool has_bootfile)
> >       for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
> >               if (opts[o].state == OPT_UNSET || opts[o].sent)
> >                       continue;
> > -
> >               if (!has_bootfile &&
> >                       fill_one(m->file, sizeof(m->file) - 1, o,
> > -                              &file_off))
> > +                              &file_off))
> > +                     if (!is_concat_opt(o))
> >                               debug("DHCP: skipping option %i"
> >                                     " (overload full)", o);
> >       }
> >
> > +     /* RFC 3396: split concatenation-requiring options that didn't fit
> > +      * as a single option.  Split order: options, file, sname.
> > +      */
> > +     for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
> > +             size_t file_cap, sname_cap, total, written;
> > +
> > +             if (opts[o].state == OPT_UNSET || opts[o].sent ||
> > +                 !is_concat_opt(o))
> > +                     continue;
> > +
> > +             sname_cap = sizeof(m->sname) - 1 > (size_t)sname_off ?
> > +                         sizeof(m->sname) - 1 - sname_off : 0;
> > +
> > +             if (has_bootfile || sizeof(m->file) - 1 <=
> (size_t)file_off)
> > +                     file_cap = 0;
> > +             else
> > +                     file_cap = sizeof(m->file) - 1 - file_off;
> > +
> > +             total = (size > (size_t)offset ? size - offset : 0)
> > +                     + file_cap + sname_cap;
> > +
> > +             if (total < (size_t)opts[o].slen) {
> >
>
> This doesn't account for the extra 2 bytes per chunk of option code
> and length, right?
>

yes, I missed it by chance while calculating the total available bytes.

>
> +                       debug("DHCP: skipping option %i (no space to
> split)",
> > +                           o);
> > +                     continue;
> > +             }
> > +
> > +             written = 0;
> > +             written += fill_split(m->o, size, o, &offset,
> > +                                   opts[o].s, opts[o].slen);
> > +             if (written < (size_t)opts[o].slen && !has_bootfile)
> > +                     written += fill_split(m->file,
> > +                                           sizeof(m->file) - 1, o,
> > +                                           &file_off,
> > +                                           opts[o].s + written,
> > +                                           opts[o].slen - written);
> > +             if (written < (size_t)opts[o].slen)
> > +                     fill_split(m->sname,
> > +                                sizeof(m->sname) - 1, o,
> > +                                &sname_off,
> > +                                opts[o].s + written,
> > +                                opts[o].slen - written);
> > +             opts[o].sent = 1;
>
> Since you didn't account for the per-chunk header, I think you need to
> check for written < opts[o].slen and not set sent in that case.
>
> > +     }
> > +
> >       if (sname_off) {
> >               m->sname[sname_off] = 255;
> >               *overload |= DHCP_OVERLOAD_SNAME;
> > --
> > 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
>


-- 
Anshu

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