From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=RAj0WrbH; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 6B8B05A0269 for ; Thu, 10 Sep 2026 09:04:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1789023886; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3ftrxUsTfUjzYPQ1EpSy2V6QEtqOZCAiYbo7jL5tdFc=; b=RAj0WrbHFe70H5UA/hXDafZMXhcJxKRSxS0rw6qFk8JshwxiL/m/CMQWrNrta1jX4PdQxj 338UfpRFonnd7WWvLldBy4wineQZU48WNjTCy9pW5ataCMXa2s+WoLa1wMjGG6H770a5tz 4Nmq/BSStMfjziNY8M2AMSRfA1NTmKo= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-616-TKJZD9U6NxqmSM5ztADy6Q-1; Thu, 10 Sep 2026 03:04:43 -0400 X-MC-Unique: TKJZD9U6NxqmSM5ztADy6Q-1 X-Mimecast-MFC-AGG-ID: TKJZD9U6NxqmSM5ztADy6Q_1789023882 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-49d1a778b9dso28430225e9.0 for ; Thu, 10 Sep 2026 00:04:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1789023882; x=1789628682; h=date:content-transfer-encoding:content-type:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to:content-type; bh=3ftrxUsTfUjzYPQ1EpSy2V6QEtqOZCAiYbo7jL5tdFc=; b=tZOO/ULyTVRCx1UyZ+Vx8zYa6SK8iGD6d2z4WYy977YwjPlhtRst5er795kDYzetbK KN+FQSatOcxXhHdDPIVfYOlDZ731driGfsZkBPnjF5naXP9Wg3+bseejm2P7w80aeFJd 2DyLO/xm7waaBF93b8QLCKeliVNywnsqW/hNc43xlgZ8tNUzYTuIg6UjxnMNRj57IHry DglFXyxCe7xYFzs/J2WfbwzAj2RhewbKAUI2KwIaEbo4YT7NQ2sTTuL2mXX79OUNSuCL ooVQZgZILShFCGqW6hzWNVbQ5m04dlP+xAjZF9nfOCfd1FqqjPvgSB7Q1LQlBO04yemQ KM0w== X-Gm-Message-State: AFuF++k3PFt9mbiN2Gasz39QdasuE2iKS1o6H3lPH5nw+w8xyLQP9lYQ nE6TZJ3s26p36MAt6D5AmD4NLFn3Y9Da7vVhMnnryxGJQJDdb8b3ktPs0bdAmxGxNcG9otpedrt oI0wKmHA7h22YLMU47SYm9zRnNnjP3YVl4EwrvFvyACmpR84hcVq5bg== X-Gm-Gg: AYBFou2xjUVrUdR4Nrnf9AtbesuyrR7RVuPjoNvsCdzwVhc+rImzFcjQwyhf1XKmChs 55DfxAGn/QL1F3ZxedgwPwEku0GwtaNyG3KAb+AP3kGgcVZhi2Fs+uzOo1RP9Oj4AT2pU52QkDq iQ8l9UsQk+5qEwxwBKrbhn6CvSUBmDEl+Xv8F6mWuuaizhYdEfWMmqNU7jbAjac2Us5IiSqoBxX 8ZMbKNpS4f/r13CNxNzqjUi/fSrsRNdMlfBeR3XBhtjAvu9buzG4LSoIemon2kDPm71A33Opn1T eUQHw1o3V48pqbErrswFIKkb2jnd3F9J2WMV1QsBRC72kAzeMhBJkzsw5qMM/GDPb3Q9hEIhb3m wuaZZ1vlaDVk= X-Received: by 2002:a05:600c:45d5:b0:493:f140:c3fb with SMTP id 5b1f17b1804b1-49d258f1d5cmr96812655e9.7.1789023882276; Thu, 10 Sep 2026 00:04:42 -0700 (PDT) X-Received: by 2002:a05:600c:45d5:b0:493:f140:c3fb with SMTP id 5b1f17b1804b1-49d258f1d5cmr96811425e9.7.1789023881762; Thu, 10 Sep 2026 00:04:41 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49db038401fsm17528365e9.13.2026.09.10.00.04.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Sep 2026 00:04:41 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH v6 6/6] dhcp: Add RFC 3396 option splitting for concatenation-requiring options Message-ID: <20260910090439.04e77f10@elisabeth> In-Reply-To: <20260824134436.282300-7-anskuma@redhat.com> References: <20260824134436.282300-1-anskuma@redhat.com> <20260824134436.282300-7-anskuma@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Thu, 10 Sep 2026 09:04:40 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: JNDRL3IFsowvnfBzZ8C55LykcE-Ejxpj0PtfZwOjvWo_1789023882 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: A2ZI2R4C7NBBJ7N5NJ2KPJDNFZ2SQCEL X-Message-ID-Hash: A2ZI2R4C7NBBJ7N5NJ2KPJDNFZ2SQCEL X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, david@gibson.dropbear.id.au, jmaloy@redhat.com, lvivier@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon, 24 Aug 2026 19:14:34 +0530 Anshu Kumari wrote: > Implement option splitting per RFC 3396 for options that may exceed > 255 bytes. A new concat_req[] lookup table marks options requiring > concatenation (currently option 81, Client FQDN per RFC 4702). > > The opts[].s buffer is resized from 255 to 497 bytes > (OPT_CONCAT_MAX) 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() calls fill_split() to split 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 > --- > v6: > - Merged v5 patches 6/7 and 7/7 into a single patch. > - Replaced DHCP_OPT_STR_CONCAT enum value and is_concat_opt() > helper with a concat_req[] boolean lookup table. > - Used MIN() macro instead of ternary for chunk size. > - Fixed space calculation to account for 2-byte code+length > overhead per chunk. > > 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 (497) bytes > - Add /* fallthrough */ between DHCP_OPT_STR and DHCP_OPT_STR_CONCAT case > --- > dhcp.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 98 insertions(+), 1 deletion(-) > > diff --git a/dhcp.c b/dhcp.c > index 43ce133..eda04d1 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -35,6 +35,11 @@ > #include "dhcp.h" > #include "parse.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 497 I think I already suggested this on an earlier revision: instead of just writing 497, for documentation, you could define this after OPT_MAX and make it clear where this number comes from. It should be OPT_MAX - 2 + 64 - 2 + 128 - 2... which gives me 493, hmm. Is OPT_MAX not correct anymore for some reason? Or is 497 too much? > + > /** > * enum opt_state - DHCP option state > * @OPT_UNSET: Option not configured > @@ -59,7 +64,7 @@ enum opt_state { > struct opt { > int sent; > int slen; > - uint8_t s[255]; > + uint8_t s[OPT_CONCAT_MAX]; clang-tidy reports: /home/sbrivio/passt/dhcp.c:64:8: error: Excessive padding in 'struct opt' (4 padding bytes, where 0 is optimal). Optimal fields order: sent, slen, clen, state, c, s, consider reordering the fields or adding explicit padding members [clang-analyzer-optin.performance.Padding,-warnings-as-errors] 64 | struct opt { | ~~~~~~~^~~~~ 65 | int sent; | ~~~~~~~~~ 66 | int slen; | ~~~~~~~~~ 67 | uint8_t s[OPT_CONCAT_MAX]; | ~~~~~~~~~~~~~~~~~~~~~~~~~~ 68 | int clen; | ~~~~~~~~~ 69 | uint8_t c[255]; | ~~~~~~~~~~~~~~~ 70 | enum opt_state state; | ~~~~~~~~~~~~~~~~~~~~~ 71 | }; | ~ that is, by keeping 's' in the middle, we unnecessarily waste space (where struct opt is not used as an array element, at least). It should be moved at the end. > int clen; > uint8_t c[255]; > enum opt_state state; > @@ -215,6 +220,13 @@ static const enum dhcp_opt_type dhcp_opt_types[] = { > [252] = DHCP_OPT_STR, /* WPAD URL */ > }; > > +/** > + * concat_req - Options requiring RFC 3396 concatenation, indexed by code By the way, as far as I know, kerneldoc doesn't really specify a documentation style for stand-alone global variables / arrays like this. This is fine as well, but generally we simply use something that's not kerneldoc, such as: /* Options ... */ static const bool ... > + */ > +static const bool concat_req[256] = { > + [81] = true, /* Client FQDN (RFC 4702, Section 2) */ > +}; > + > /** > * dhcp_opt_parse() - Parse a DHCP option value > * @code: DHCP option code > @@ -309,6 +321,9 @@ static int dhcp_opt_parse(uint8_t code, const char *str, > case DHCP_OPT_STR: > slen = strlen(str); > > + if (!concat_req[code] && slen > 255) > + return -1; > + > if (slen >= buf_len) > return -1; > > @@ -442,6 +457,40 @@ enum dhcp_overload { > DHCP_OVERLOAD_SNAME = 2, > }; > > +/** > + * 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 = MIN(remaining, avail); > + 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 > @@ -495,6 +544,54 @@ static int fill(struct msg *m, enum dhcp_overload *overload, bool has_bootfile) > } > } > > + /* 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++) { This could use foreach() (or foreach_opt()). > + size_t file_cap, sname_cap, total, written; > + > + if (opts[o].state == OPT_UNSET || opts[o].sent || > + !concat_req[o]) > + continue; > + > + sname_cap = sizeof(m->sname) - 1 > (size_t)sname_off ? If you initialise sname_cap to 0 above, then, with the same amount of lines, you could write (more readable I think): if (sizeof(m->sname) - 1 > (size_t)sname_off) sname_cap = sizeof(m->sname) - 1 - 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 - 2 : 0) > + + (file_cap > 2 ? file_cap - 2 : 0) > + + (sname_cap > 2 ? sname_cap - 2 : 0); We always write operators (where possible) at the end of the previous line, that is: total = (size > (size_t)offset ? size - offset - 2 : 0) + file_cap > 2 ? file_cap - 2 : 0) + sname_cap > 2 ? sname_cap - 2 : 0); > + > + if (total < (size_t)opts[o].slen) { > + 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) Curly brackets preferred (here and above). > + written += fill_split(m->sname, > + sizeof(m->sname) - 1, o, > + &sname_off, > + opts[o].s + written, > + opts[o].slen - written); > + > + if (written >= (size_t)opts[o].slen) > + opts[o].sent = 1; Sorry, this didn't occur to me in earlier review rounds: if we couldn't write the whole concatenation-requiring option, we just print a debug message, and that's it: we'll leave a truncated option in the message, instead of skipping it altogether, which might cause all sorts of issues (the value at this point is wrong). To avoid this, we should calculate if the option fits at all, first, and then write it to buffers. Maybe, to keep the implementation simple, fill_split() could have a 'dry_run' parameter, and, if it's set, it just checks stuff without actually writing anything. Then, call fill_split() with dry_run set, first, and if everything succeeds call it with dry_run unset? > + } > + > /* 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) -- Stefano