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

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 24 Aug 2026 19:14:34 +0530
Anshu Kumari <anskuma@redhat.com> wrote:

> Implement option splitting per RFC 3396 for options that may exceed
> 255 bytes.  A new concat_req[] lookup table marks options requiring
> concatenation (currently option 81, Client FQDN per RFC 4702).
> 
> The opts[].s buffer is resized from 255 to 497 bytes
> (OPT_CONCAT_MAX) 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() calls fill_split() to split 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>
> ---
> v6:
> - Merged v5 patches 6/7 and 7/7 into a single patch.
> - Replaced DHCP_OPT_STR_CONCAT enum value and is_concat_opt()
>   helper with a concat_req[] boolean lookup table.
> - Used MIN() macro instead of ternary for chunk size.
> - Fixed space calculation to account for 2-byte code+length
>   overhead per chunk.
> 
> 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 (497) bytes
>   - Add /* fallthrough */ between DHCP_OPT_STR and DHCP_OPT_STR_CONCAT case
> ---
>  dhcp.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 43ce133..eda04d1 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -35,6 +35,11 @@
>  #include "dhcp.h"
>  #include "parse.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	497

I think I already suggested this on an earlier revision: instead of
just writing 497, for documentation, you could define this after
OPT_MAX and make it clear where this number comes from. It should be
OPT_MAX - 2 + 64 - 2 + 128 - 2... which gives me 493, hmm. Is OPT_MAX
not correct anymore for some reason? Or is 497 too much?

> +
>  /**
>   * enum opt_state - DHCP option state
>   * @OPT_UNSET:		Option not configured
> @@ -59,7 +64,7 @@ enum opt_state {
>  struct opt {
>  	int sent;
>  	int slen;
> -	uint8_t s[255];
> +	uint8_t s[OPT_CONCAT_MAX];

clang-tidy reports:

/home/sbrivio/passt/dhcp.c:64:8: error: Excessive padding in 'struct opt' (4 padding bytes, where 0 is optimal). Optimal fields order: sent, slen, clen, state, c, s, consider reordering the fields or adding explicit padding members [clang-analyzer-optin.performance.Padding,-warnings-as-errors]
   64 | struct opt {
      | ~~~~~~~^~~~~
   65 |         int sent;
      |         ~~~~~~~~~
   66 |         int slen;
      |         ~~~~~~~~~
   67 |         uint8_t s[OPT_CONCAT_MAX];
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |         int clen;
      |         ~~~~~~~~~
   69 |         uint8_t c[255];
      |         ~~~~~~~~~~~~~~~
   70 |         enum opt_state state;
      |         ~~~~~~~~~~~~~~~~~~~~~
   71 | };
      | ~

that is, by keeping 's' in the middle, we unnecessarily waste space
(where struct opt is not used as an array element, at least). It should
be moved at the end.

>  	int clen;
>  	uint8_t c[255];
>  	enum opt_state state;
> @@ -215,6 +220,13 @@ static const enum dhcp_opt_type dhcp_opt_types[] = {
>  	[252] = DHCP_OPT_STR,		/* WPAD URL */
>  };
>  
> +/**
> + * concat_req - Options requiring RFC 3396 concatenation, indexed by code

By the way, as far as I know, kerneldoc doesn't really specify a
documentation style for stand-alone global variables / arrays like
this. This is fine as well, but generally we simply use something
that's not kerneldoc, such as:

/* Options ... */
static const bool ...

> + */
> +static const bool concat_req[256] = {
> +	[81] = true,	/* Client FQDN (RFC 4702, Section 2) */
> +};
> +
>  /**
>   * dhcp_opt_parse() - Parse a DHCP option value
>   * @code:	DHCP option code
> @@ -309,6 +321,9 @@ static int dhcp_opt_parse(uint8_t code, const char *str,
>  	case DHCP_OPT_STR:
>  		slen = strlen(str);
>  
> +		if (!concat_req[code] && slen > 255)
> +			return -1;
> +
>  		if (slen >= buf_len)
>  			return -1;
>  
> @@ -442,6 +457,40 @@ enum dhcp_overload {
>  	DHCP_OVERLOAD_SNAME = 2,
>  };
>  
> +/**
> + * 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 = MIN(remaining, avail);
> +	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
> @@ -495,6 +544,54 @@ static int fill(struct msg *m, enum dhcp_overload *overload, bool has_bootfile)
>  		}
>  	}
>  
> +	/* 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++) {

This could use foreach() (or foreach_opt()).

> +		size_t file_cap, sname_cap, total, written;
> +
> +		if (opts[o].state == OPT_UNSET || opts[o].sent ||
> +		    !concat_req[o])
> +			continue;
> +
> +		sname_cap = sizeof(m->sname) - 1 > (size_t)sname_off ?

If you initialise sname_cap to 0 above, then, with the same amount of
lines, you could write (more readable I think):

	if (sizeof(m->sname) - 1 > (size_t)sname_off)
		sname_cap = sizeof(m->sname) - 1 - 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 - 2 : 0)
> +			+ (file_cap > 2 ? file_cap - 2 : 0)
> +			+ (sname_cap > 2 ? sname_cap - 2 : 0);

We always write operators (where possible) at the end of the previous
line, that is:

		total = (size > (size_t)offset ? size - offset - 2 : 0) +
			file_cap > 2 ? file_cap - 2 : 0) +
			sname_cap > 2 ? sname_cap - 2 : 0);

> +
> +		if (total < (size_t)opts[o].slen) {
> +			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)

Curly brackets preferred (here and above).

> +			written += fill_split(m->sname,
> +					      sizeof(m->sname) - 1, o,
> +					      &sname_off,
> +					      opts[o].s + written,
> +					      opts[o].slen - written);
> +
> +		if (written >= (size_t)opts[o].slen)
> +			opts[o].sent = 1;

Sorry, this didn't occur to me in earlier review rounds: if we couldn't
write the whole concatenation-requiring option, we just print a debug
message, and that's it: we'll leave a truncated option in the message,
instead of skipping it altogether, which might cause all sorts of
issues (the value at this point is wrong).

To avoid this, we should calculate if the option fits at all, first,
and then write it to buffers. Maybe, to keep the implementation simple,
fill_split() could have a 'dry_run' parameter, and, if it's set, it
just checks stuff without actually writing anything.

Then, call fill_split() with dry_run set, first, and if everything
succeeds call it with dry_run unset?

> +	}
> +
>  	/* Report any options that could not be sent */
>  	for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
>  		if (opts[o].state != OPT_UNSET && !opts[o].sent)

-- 
Stefano