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

On Mon, Jul 20, 2026 at 9:54 AM David Gibson 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 --- 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