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

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 --- 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;