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=XgGWYEwV; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id CCF935A026E for ; Thu, 08 Oct 2026 00:06:44 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1791410803; 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=6REXBj68fMemvRx5m+zVhSk3FkuipKqIJCEWk7lGV3E=; b=XgGWYEwVk9+cp7rNpbAfPJfVwBBrlHGPRjtQ2o5f15agd60hO8GCCqsVvk7x5JA9ivi/iJ mmrNQ/jhPtNSoHkGlgBpxTNHTwyRS+yMrwPKHaL2dNhoYr942wxP/wrMum+sEL+Lke6a+A lE97YrUw9agjLH/8PZqbw3xEE8N0eLs= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-75-D8-xeYw_PPavvV92n46quQ-1; Wed, 7 Oct 2026 22:06:41 +0000 X-MC-Unique: D8-xeYw_PPavvV92n46quQ-1 X-Mimecast-MFC-AGG-ID: D8-xeYw_PPavvV92n46quQ_1791410800 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-48c4f6f6691so3811059f8f.2 for ; Wed, 07 Oct 2026 15:06:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1791410800; x=1792015600; 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=6REXBj68fMemvRx5m+zVhSk3FkuipKqIJCEWk7lGV3E=; b=DxPS4ytYMJxQeOuz8+TYXWE6xGbj1Mv3joBVUeHQF1zvBMo4RuY4H2irw2v+b7lBnl Il0Q7VH+RvsiS7jyUHVuM40N7U/KXDShBFlYnwvVYBFyalY64udHMSngibCnYzpPp279 x+bv9loEz26RPi6aR+vErh3oNqr9Cwom4I/tt1dJZK63OeBHsoMt8pvCLmdOmA7kWLRa YoHn6RmK9ixx7r3qvsGf1g0c/pgYvPE5JFequl3pW2xFNt59H1PMoBYeW9PmOvtY4LXr /uyGwwD6mfMde7OpfkRM1dcVD4cGTy/1m+DTpT44m95JDM9P6WCN1dxRKWQ+2aV+Ftfm Bx3w== X-Gm-Message-State: AFq9FYI+TTj0mqMGBhWjgBfngMIqg/w7FFVQNoIJf5V6EXZOCUqA5j42 KTuDg8VUKBDU3KcqYEk3KE2PGU4tTXfXVFuuy93zUgyCeushadF/I5w67lmJjrF21vFn0vW1Cjp TKbE5EZZ1aUuHvpG06yA/dtQf+yws++HupEyKmiX0blysqI9kHrZwCM3pqOgPqCq3LHXZGOK8hl SFwqh2g6BRAqFBTfxQGDoxTgApF3FILQRpn490 X-Gm-Gg: AYBFou0qu0sO52HSlycQUWjWy65nxTFek6VKfCmyBOWnj5r58+oiUIMn58TOQSxDgL0 M20y66EwzUEwcc48+1yADpAy03r2tdS9JE8RslJYzMzRhVNTm6/z2LTBk7ntiABtn3qOe2PaR0+ RP/GPVJIy7fUdYL7HmT1f7v3IRF1wWy74s+4PoXE2UwGGJ3krYM20V9PrUchRHW7RgJHNTJkhxY trFa1JVwkH7779IXyKMgqlYUp4olH6H6u/2ud6acn6dCRwkSLKq/uNIJESy0SMcjhbc1RZmnwCz P9n1UfapdRMDRswWaTKoPWyoFSspX/Hfh2V5MV+ESWfhp9ZIRS+BjXiwHjbAcxKexLafFtZs89V irhotbF4mnsWi/DhtamDM4W2kvSc= X-Received: by 2002:a5d:6a86:0:b0:486:f3c0:c26b with SMTP id ffacd0b85a97d-48c7289763emr5165700f8f.57.1791410800200; Wed, 07 Oct 2026 15:06:40 -0700 (PDT) X-Received: by 2002:a5d:6a86:0:b0:486:f3c0:c26b with SMTP id ffacd0b85a97d-48c7289763emr5165659f8f.57.1791410799505; Wed, 07 Oct 2026 15:06:39 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-48c71d13a01sm8132674f8f.29.2026.10.07.15.06.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Oct 2026 15:06:38 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH v7 7/7] dhcp: Add RFC 3396 option splitting for concatenation-requiring options Message-ID: <20261008000637.23e2f17b@elisabeth> In-Reply-To: <20261001131602.653553-8-anskuma@redhat.com> References: <20261001131602.653553-1-anskuma@redhat.com> <20261001131602.653553-8-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, 08 Oct 2026 00:06:38 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: jTYa4GEBzxoM9MP7LQeXfgWyEbg5-obCLMyV9NzzGTA_1791410800 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 2PAMXUH6WWUF5ZECGQAE6KKJHVTRUGUO X-Message-ID-Hash: 2PAMXUH6WWUF5ZECGQAE6KKJHVTRUGUO 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, 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 Thu, 1 Oct 2026 18:46:00 +0530 Anshu Kumari wrote: > Implement option splitting per RFC 3396 for options that may exceed > 255 bytes. A 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 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. Each split chunk is capped at 255 bytes per the DHCP > option length field limit. > > Link: https://bugs.passt.top/show_bug.cgi?id=192 > Signed-off-by: Anshu Kumari > --- > v7: > - Capped each split chunk at 255 bytes: > chunk = MIN(MIN(remaining, avail), 255) to prevent > overflow in the uint8_t length field > - Replaced inline space calculation with a dry-run pass > via fill_split_areas() > - Moved OPT_MAX and OPT_CONCAT_MAX definitions above struct opt. > - Reordered struct opt fields to place s[OPT_CONCAT_MAX] > - Added dry_run parameter to fill_split() > > 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 | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 142 insertions(+), 18 deletions(-) > > diff --git a/dhcp.c b/dhcp.c > index 63ac8e3d..990b650d 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -35,6 +35,19 @@ > #include "dhcp.h" > #include "parse.h" > > +#define OPT_MIN 60 /* RFC 951 */ > + > +/* Total option size (excluding end option) is 576 (RFC 2131), minus > + * offset of options (268), minus end option (1). > + */ > +#define OPT_MAX 307 > + > +/* RFC 3396: maximum option data that can be split across options field > + * (OPT_MAX - 2), file field (64 - 2), and sname field (128 - 2), > + * minus code+length overhead per portion. That doesn't match the define below (which is the correct one, I think). If you additionally subtract at the end, from all that, code and length bytes for each section, you would end up with an extra -6 term, which isn't needed. > + */ > +#define OPT_CONCAT_MAX (OPT_MAX - 2 + 64 - 2 + 128 - 2) > + > /** > * enum opt_conf - DHCP option configuration > * @OPT_UNSET: Option not configured > @@ -51,18 +64,18 @@ enum opt_conf { > * struct opt - DHCP option > * @sent: Convenience flag, set while filling replies > * @slen: Length of option defined for server > - * @s: Option payload from server > * @clen: Length of option received from client, -1 if not received > * @c: Option payload from client > * @conf: Option configuration (unset, default, or user) > + * @s: Option payload from server > */ > struct opt { > int sent; > int slen; > - uint8_t s[255]; > int clen; > uint8_t c[255]; > enum opt_conf conf; > + uint8_t s[OPT_CONCAT_MAX]; As I mentioned, clang-tidy (make clang-tidy) suggests rearranging this in a way that doesn't need padding. > }; > > static struct opt opts[256]; > @@ -79,32 +92,23 @@ static struct opt opts[256]; > #define DHCPINFORM 8 > #define DHCPFORCERENEW 9 > > -#define OPT_MIN 60 /* RFC 951 */ > - > -/* Total option size (excluding end option) is 576 (RFC 2131), minus > - * offset of options (268), minus end option (1). > - */ > -#define OPT_MAX 307 > - > /** > * dhcp_init() - Initialise DHCP options > */ > void dhcp_init(void) > { > if (opts[1].conf != OPT_USER) /* Mask */ > - opts[1] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, OPT_DEFAULT }; > + opts[1] = (struct opt) { 0, 4, 0, { 0 }, OPT_DEFAULT, { 0 } }; > if (opts[3].conf != OPT_USER) /* Router */ > - opts[3] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, OPT_DEFAULT }; > + opts[3] = (struct opt) { 0, 4, 0, { 0 }, OPT_DEFAULT, { 0 } }; > if (opts[51].conf != OPT_USER) { /* Lease time */ > - opts[51] = (struct opt) { 0, 4, { 0xff, 0xff, 0xff, 0xff }, > - 0, { 0 }, OPT_DEFAULT }; > + opts[51] = (struct opt) { 0, 4, 0, { 0 }, OPT_DEFAULT, > + { 0xff, 0xff, 0xff, 0xff } }; > } > /* Type */ > - opts[53] = (struct opt) { 0, 1, { 0 }, 0, { 0 }, OPT_DEFAULT }; > - if (opts[54].conf != OPT_USER) { /* Server ID */ > - opts[54] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, > - OPT_DEFAULT }; > - } > + opts[53] = (struct opt) { 0, 1, 0, { 0 }, OPT_DEFAULT, { 0 } }; > + if (opts[54].conf != OPT_USER) /* Server ID */ > + opts[54] = (struct opt) { 0, 4, 0, { 0 }, OPT_DEFAULT, { 0 } }; > } > > /** > @@ -211,6 +215,11 @@ static const enum dhcp_opt_type dhcp_opt_types[] = { > [252] = DHCP_OPT_STR, /* WPAD URL */ > }; > > +/* Options requiring RFC 3396 concatenation, indexed by code */ > +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 > @@ -308,6 +317,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; > > @@ -441,6 +453,45 @@ static void fill_one(uint8_t *buf, size_t size, int o, int *offset) > *offset += slen; > } > > +/** > + * 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 > + * @dry_run: If true, calculate size without writing to @buf > + * > + * 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, > + bool dry_run) > +{ > + size_t avail, chunk; > + > + if (*offset + 2 >= (int)size) > + return 0; > + > + avail = size - *offset - 2; > + chunk = MIN(MIN(remaining, avail), 255); > + if (!chunk) > + return 0; > + > + if (!dry_run) { > + buf[*offset] = o; > + buf[*offset + 1] = chunk; > + } > + *offset += 2; > + > + if (!dry_run) > + memcpy(buf + *offset, data, chunk); > + *offset += chunk; > + > + return chunk; > +} > + > /** > * enum dhcp_overload - DHCP option overload values (RFC 2132, Section 9.3) > * @DHCP_OVERLOAD_NONE: No overload > @@ -453,6 +504,43 @@ enum dhcp_overload { > DHCP_OVERLOAD_SNAME = 2, > }; > > +/** > + * fill_split_areas() - Write a split option across options, file, and sname areas > + * @m: Message to write into > + * @opt_size: Usable size of the options area > + * @o: Option number (code) > + * @opt_off: Current offset within options area, updated on write > + * @file_off: Current offset within file area, updated on write > + * @sname_off: Current offset within sname area, updated on write > + * @has_bootfile: If true, skip the file field > + * @dry_run: If true, calculate size without writing > + * > + * Return: total data bytes written across all areas > + */ > +static size_t fill_split_areas(struct msg *m, size_t opt_size, int o, > + int *opt_off, int *file_off, int *sname_off, > + bool has_bootfile, bool dry_run) > +{ > + size_t written = 0; No need to initialise this to 0 and then increment it unconditionally below, I think it's a bit confusing. > + > + written += fill_split(m->o, opt_size, o, opt_off, > + opts[o].s, opts[o].slen, dry_run); > + 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, dry_run); > + } > + if (written < (size_t)opts[o].slen) { > + written += fill_split(m->sname, sizeof(m->sname) - 1, o, > + sname_off, > + opts[o].s + written, > + opts[o].slen - written, dry_run); > + } > + > + return written; > +} > + > /** > * fill() - Fill options in message, with overload into file/sname if needed > * @m: Message to fill > @@ -504,6 +592,42 @@ static int fill(struct msg *m, bool has_bootfile) > } > } > > + /* RFC 3396: split concatenation-requiring options that didn't fit > + * as a single option. Split order: options, file, sname. > + */ > + foreach_opt(o) { This could also use the foreach_unsent_opt() iterator I was suggesting for 6/7. > + int dry_off, dry_file_off, dry_sname_off; > + size_t total, written; > + > + if (opts[o].conf == OPT_UNSET || opts[o].sent || > + !concat_req[o]) > + continue; > + > + /* Dry run: verify the option fits across all areas */ > + dry_off = offset; > + dry_file_off = file_off; > + dry_sname_off = sname_off; > + > + total = fill_split_areas(m, size, o, > + &dry_off, &dry_file_off, > + &dry_sname_off, > + has_bootfile, true); > + > + if (total < (size_t)opts[o].slen) { > + debug("DHCP: skipping option %i (no space to split)", Splitting itself doesn't need extra space, it's simply that we don't have space for the option (not even if split). Sorry, I missed this on v6. > + o); > + continue; > + } > + > + /* Actual write */ > + written = fill_split_areas(m, size, o, > + &offset, &file_off, &sname_off, > + has_bootfile, false); > + > + if (written >= (size_t)opts[o].slen) > + opts[o].sent = 1; > + } > + > /* Report any options that could not be sent */ > foreach_opt(o) { > if (opts[o].conf != OPT_UNSET && !opts[o].sent) -- Stefano