Re: [PATCH v6 3/6] dhcp: Add option overload

[Laurent Vivier <lvivier@redhat.com>]

On 8/24/26 15:44, 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>
> ---
> v6:
>    - Changed fill_one() return type from bool to void.
>    - Replaced per-call-site debug messages with a single
>      reporting loop at end of fill().
>    - Added explicit numeric values to enum dhcp_overload.
>    - Removed temporary buffer for boot file copy,
>      copy directly into reply.file.
>    - Fixed mixed code and declarations.
>    - Fixed indentation and whitespace issues.
>    - Added curly brackets for multi-line if clauses
> 
> 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 | 98 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 81 insertions(+), 17 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 321968a..02c7744 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -146,16 +146,14 @@ struct msg {
>    * @size:	Usable size of @buf (excluding end marker)
>    * @o:		Option number
>    * @offset:	Current offset within @buf, updated on insertion
> - *
> - * Return: false if @buf has space to write the option, true otherwise
>    */
> -static bool fill_one(uint8_t *buf, size_t size, int o, int *offset)
> +static void fill_one(uint8_t *buf, size_t size, int o, int *offset)
>   {
>   	size_t slen = opts[o].slen;
>   
>   	/* If we don't have space to write the option, then just skip */
>   	if (*offset + 2 /* code and length of option */ + slen > size)
> -		return true;
> +		return;
>   
>   	buf[*offset] = o;
>   	buf[*offset + 1] = slen;
> @@ -167,19 +165,37 @@ static bool fill_one(uint8_t *buf, size_t size, int o, int *offset)
>   
>   	opts[o].sent = 1;
>   	*offset += slen;
> -	return false;
>   }
>   

changing the return type of fill_one() and removing debug information should be in a 
separate patch. It's hard to understand the change when we focus on the overload option...

The patch can also explains that we rely now on opts[].sent to know if the option has been 
written in buf.

>   /**
> - * fill() - Fill options in message
> - * @m:		Message to fill
> + * enum dhcp_overload - DHCP option overload values (RFC 2132, Section 9.3)
> + * @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  = 0,
> +	DHCP_OVERLOAD_FILE  = 1,
> +	DHCP_OVERLOAD_SNAME = 2,
> +};
> +
> +/**
> + * 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;
> +	/* Reserve 3 bytes for option 52 (overload) if needed */
> +	size_t size = OPT_MAX - 3;
>   	int i, o, offset = 0;
>   
> +	*overload = DHCP_OVERLOAD_NONE;

Why overload is a parameter? it is only used inside the function and the set value is 
never read by the caller.

> +
>   	for (o = 0; o < 255; o++)
>   		opts[o].sent = 0;
>   
> @@ -188,20 +204,54 @@ 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))
> -		if (fill_one(m->o, OPT_MAX, 53, &offset))
> -			debug("DHCP: skipping option 53");
> +		fill_one(m->o, size, 53, &offset);
>   
>   	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);
> +	}
> +
> +	if (!has_bootfile) {
> +		for (o = 0; (size_t)o < ARRAY_SIZE(opts); o++) {
> +			if (opts[o].state == OPT_UNSET || opts[o].sent)
> +				continue;
> +			fill_one(m->file, sizeof(m->file) - 1, o, &file_off);
> +		}
> +	}
> +
> +	/* 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)
> +			debug("DHCP: skipping option %i", 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;
> +	}
> +
> +	if (*overload) {
> +		m->o[offset++] = 52;
> +		m->o[offset++] = 1;
> +		m->o[offset++] = *overload;
>   	}
>   
>   	m->o[offset++] = 255;
> @@ -315,6 +365,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;
> @@ -325,6 +376,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>   	const struct udphdr *uh;
>   	struct msg m_storage;
>   	struct msg const *m;
> +	bool has_bootfile;
>   	struct msg reply;
>   	unsigned int i;
>   
> @@ -486,10 +538,22 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>   		}
>   	}
>   
> -	if (!c->no_dhcp_dns_search)
> -		opt_set_dns_search(c, sizeof(m->o));
> +	if (!c->no_dhcp_dns_search) {
> +		/* 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);
> +
> +	dlen = offsetof(struct msg, o) + fill(&reply, &overload, has_bootfile);
>   
> -	dlen = offsetof(struct msg, o) + fill(&reply);
> +	if (has_bootfile)
> +		memcpy(reply.file, opts[67].s, opts[67].slen);

Perhaps the setting of reply.file could me merged in PATCH 5, and move this patch after 
PATCH 5?


>   
>   	if (m->flags & FLAG_BROADCAST)
>   		dst = in4addr_broadcast;