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

[David Gibson <david@gibson.dropbear.id.au>]

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

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.

>   */
>  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?

+			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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]