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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 17 Jun 2026 18:52:36 +0530
Anshu Kumari <anskuma@redhat.com> 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 */

See your own enum dhcp_opt_type in 3/4 of this series for an example of
how to document enum in kerneldoc-style, or my review of 3/6 from your
first revision:

  https://archives.passt.top/passt-dev/20260520023932.6a7e3537@elisabeth/

> +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);
> +	}
> +
> +	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 {
> +		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);
> +		}
> +	}

You could merge these two:

	for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
		if (opts[o].slen == -1 || opts[o].sent)
			continue;

		if (!has_bootfile ||
		    fill_one(m->file, sizeof(m->file) - 1, o, &file_off))
			debug("DHCP: skipping option %i (overload full)", o);
	}

or, maybe a bit silly but probably more readable:

	size_t bootfile_size = has_bootfile ? sizeof(m->file) - 1 : 0;

	[...]

	for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
		if (fill_one(m->file, bootfile_size, o, &file_off))
			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);
> +
> +	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);

Why - 3? A comment would be nice to have.

> +
> +	/* 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;

Except for pending comments from David on 3/4 and 4/4, the rest looks
good to me.

-- 
Stefano