Re: [PATCH v5 3/7] dhcp: Add option overload

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

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

On Fri, Jul 17, 2026 at 11:26:40PM +0530, Anshu Kumari 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>
> ---
> v5:
>   - enhanced enum dhcp_overload to follow kernel-doc.
>   - Inline fill_overflow() into fill()
>   - Use state-based checks instead of slen
> 
> 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 | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 79 insertions(+), 9 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index e5d89fc..39f7952 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -176,13 +176,31 @@ static bool fill_one(uint8_t *buf, size_t size, int o, int *offset)
>  }
>  
>  /**
> - * fill() - Fill options in message
> - * @m:		Message to fill
> +* enum dhcp_overload - DHCP option overload values (RFC 2132, Section 9.3)

Nit: missing space before the *.

> + * @DHCP_OVERLOAD_NONE:		No overload
> + * @DHCP_OVERLOAD_FILE:		file field carries options
> + * @DHCP_OVERLOAD_SNAME:	sname field carries options
> + */
> +enum dhcp_overload {
> +	DHCP_OVERLOAD_NONE,
> +	DHCP_OVERLOAD_FILE,
> +	DHCP_OVERLOAD_SNAME,

Nit: I'd suggest putting specific value assignments on these, even
though they're technically redundant.  It serves to make it clearer
that the specific numerical values matters, since these go "over the
wire".

> +};
> +
> +/**
> + * 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)
>  {
> +	int sname_off = 0, file_off = 0;
> +	*overload = DHCP_OVERLOAD_NONE;
> +	/* 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++)
> @@ -199,14 +217,47 @@ static int fill(struct msg *m)
>  	for (i = 0; i < opts[55].clen; i++) {
>  		o = opts[55].c[i];
>  		if (opts[o].state != OPT_UNSET)
> -			if (fill_one(m->o, OPT_MAX, o, &offset))
> -				debug("DHCP: skipping option %i", o);
> +			fill_one(m->o, size, o, &offset);
>  	}
>  
>  	for (o = 0; o < 255; o++) {
>  		if (opts[o].state != OPT_UNSET && !opts[o].sent)
> -			if (fill_one(m->o, OPT_MAX, o, &offset))
> -				debug("DHCP: skipping option %i", o);
> +			fill_one(m->o, size, o, &offset);
> +	}
> +
> +	/* Overflow unsent options into sname, then file */
> +	for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
> +		if (opts[o].state == OPT_UNSET || opts[o].sent)
> +			continue;
> +		fill_one(m->sname, sizeof(m->sname) - 1, o, &sname_off);
> +	}
> +
> +	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))
> +				debug("DHCP: skipping option %i"
> +				      " (overload full)", o);

The logic here will omit all the "skipping option" messages if
has_bootfile is true.  I think it might be cleaner to change
fill_one() to return void, and add a final pass generating the
"skipping" messages if !opts[o].sent.

> +	}
> +
> +	if (sname_off) {
> +		m->sname[sname_off] = 255;
> +		*overload |= DHCP_OVERLOAD_SNAME;
> +	}
> +
> +	if (file_off) {
> +		m->file[file_off] = 255;
> +		*overload |= DHCP_OVERLOAD_FILE;
> +	}
> +
> +
> +	if (*overload) {
> +		m->o[offset++] = 52;
> +		m->o[offset++] = 1;
> +		m->o[offset++] = *overload;
>  	}
>  
>  	m->o[offset++] = 255;
> @@ -320,6 +371,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;
> @@ -328,9 +380,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);
> @@ -492,9 +547,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));
> +		 /* 3 bytes reserved for option 52 (code, length, value) */
> +		opt_set_dns_search(c, OPT_MAX - 3);
> +
> +	/* 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;

Why copy into the 'bootfile' temporary..

> +	}
> +
> +	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);
... then out again, rather than copying directly from opts[67].s into
reply.file?

>  
>  	if (m->flags & FLAG_BROADCAST)
>  		dst = in4addr_broadcast;
> -- 
> 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 --]