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=DkSy20rk; 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 3AC225A0265 for ; Wed, 01 Jul 2026 09:51:44 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1782892303; 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=fUAcHOX7QYQyTQ7xDzExmTzuHLdbAjCgeqGPQjzPuhY=; b=DkSy20rknfelzUibaw5bmWsg6Mk1Eatw8dqW8DXbWf5kj5FPzNdgPfANqrXbeF08P5UCh4 iEUYyk68y4Tpjy98ecEX7YuFZ+r9YS2lbWRoDOwKoYjrjqDu0Cx9YEo2YTy3MN09R8MdN5 57DCvget0E/q4x6hUdLplKpDFQnlFco= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-88-3Sjm8E_aMQqgIWUGs4LX1Q-1; Wed, 01 Jul 2026 03:51:42 -0400 X-MC-Unique: 3Sjm8E_aMQqgIWUGs4LX1Q-1 X-Mimecast-MFC-AGG-ID: 3Sjm8E_aMQqgIWUGs4LX1Q_1782892301 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-4925bf70f5bso3159175e9.1 for ; Wed, 01 Jul 2026 00:51:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782892301; x=1783497101; h=date:content-transfer-encoding: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; bh=QFVVF2rWMaLqpTaw+C1jNK3PrO06QdogUtxbGdCWFmI=; b=XDu8QN0HN5cMzk2tIbKqeuVgYGJv2m+SI6YTcI7qmqgnkO3GQ7TBXmyfxb5hJTv9Ub 8lnLKAj3SXd82IvKoVDQBdhuWTFpqGtQUFlbxQEyiIUs4u167bemk5uEpkzuO9w6PlVk doBpnuCUMHo4kF+s3kqkH78ld6vPDUscFQlbryDfBksQDYGND6slRo4HmBQYLlEOfs+0 p2P1FU9EZtd7PpMkHx/qXD3S/5cS2/Bpa1432QmeQAjnyQ1u0rYxryWCSUnw2AJM86Rr 4xAAJSxTHcu6np3iDsNb4MJIWBhHcAXE37OtJJPoPTbwJj06kUHXzk5RbR0cbZwW4pxR yCOg== X-Gm-Message-State: AOJu0YyxGFWyhvOhlrwDmiDJmqcGESbiYRmYkfWBKVGvZP1xC81k2MD+ KcNhw0RRPtOWM4CvqpBxCrPkwW0w3ms9n8epz/OHphg92eOMRJmB988o3IcGdvN6Llyb6GN2kAr 3PdsoHzz0+GPCsgZbcAn9tIiEYhTsT0RUP9msv67VDRCJ6GHRs9njOw== X-Gm-Gg: AfdE7cl/KVvInPwzyUAojXVy/uLALsNNUW7517ZIxtNahwP1Mxm106dlEdKc07HBrBO saOi+tuM1k2npU24N7Ov+jcL8OMNtPLh+YmQYgS1E3eqkfBDWt3mAzIoUfJT0LXaDDuVEs2BF/o h8SQyJ47CZ1/62Jh4I42rHWvCnVEzH0fE7dHOYPV+ktNJW769ri7YBvUgsPyuybBqgpPiua/rbZ yUsY5/33saas9PQnjrEfwHnioaA+Pqkt8mI9Ghpt1NsAxoFF9iM1hVZ852X5G+kJNjPsA51cYcF azCRdbvn9yiRSawc0s4dKGoYhVhkraYLq4p/rHDyEgB/hX8FYnt8ik5rcFzscMM398q61bxt4Ot YVrfMsdGE2fojGCgnhxrI7Q== X-Received: by 2002:a05:600c:2e53:b0:493:bc92:ba9a with SMTP id 5b1f17b1804b1-493c2b53d99mr4757145e9.13.1782892300699; Wed, 01 Jul 2026 00:51:40 -0700 (PDT) X-Received: by 2002:a05:600c:2e53:b0:493:bc92:ba9a with SMTP id 5b1f17b1804b1-493c2b53d99mr4756875e9.13.1782892300157; Wed, 01 Jul 2026 00:51:40 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-493be4c9f78sm62044895e9.5.2026.07.01.00.51.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Jul 2026 00:51:39 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH v4 2/4] dhcp: Add option overload Message-ID: <20260701095137.758c02ec@elisabeth> In-Reply-To: <20260617132243.1499556-3-anskuma@redhat.com> References: <20260617132243.1499556-1-anskuma@redhat.com> <20260617132243.1499556-3-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: Wed, 01 Jul 2026 09:51:38 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 82KiyZp4WQ4w_beXMdYV3NRU99Ewc5mzNJL2573DqHk_1782892301 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: TKIV27NSTWTNE533MRRSEK43AP42RL2L X-Message-ID-Hash: TKIV27NSTWTNE533MRRSEK43AP42RL2L 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, jmaloy@redhat.com, david@gibson.dropbear.id.au, 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 Wed, 17 Jun 2026 18:52:36 +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. >=20 > 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. >=20 > Link: https://bugs.passt.top/show_bug.cgi?id=3D192 > Signed-off-by: Anshu Kumari > --- > 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. >=20 > 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) =E2=80=94 better packing and keeps file field available f= or > boot file name. > - Removed '&' from &reply.file. > - Removed '+1' from memcpy =E2=80=94 reply.file already zeroed. > - opt_set_dns_search() max_len: OPT_MAX - 3 instead of > sizeof(m->o). >=20 > v2: > - Added #define DHCP_OVERLOAD_FILE and #define DHCP_OVERLOAD_SNAME cons= tants > - Added comment documenting space reservation: /* Reserve 3 bytes for o= ption 52 */ > - Fixed DNS search length: sizeof(m->o) only, not combined with file+sn= ame > - Removed dhcp_boot references =E2=80=94 reply.file copy now reads from= opts[67] > - Used DHCP_OVERLOAD_FILE constant in reply.file guard > --- > dhcp.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 100 insertions(+), 8 deletions(-) >=20 > diff --git a/dhcp.c b/dhcp.c > index 8e3ffd6..78790d8 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -160,14 +160,79 @@ static bool fill_one(uint8_t *buf, size_t size, int= o, int *offset) > =09return false; > } > =20 > +/* RFC 2132, Section 9.3 - Option Overload */ See your own enum dhcp_opt_type in 3/4 of this series for an example of how to document enum in kerneldoc-style, or my review of 3/6 from your first revision: https://archives.passt.top/passt-dev/20260520023932.6a7e3537@elisabeth/ > +enum dhcp_overload { > +=09DHCP_OVERLOAD_NONE =3D 0, > +=09DHCP_OVERLOAD_FILE =3D 1, > +=09DHCP_OVERLOAD_SNAME =3D 2, > +}; > + > /** > - * fill() - Fill options in message > - * @m:=09=09Message to fill > + * fill_overflow() - Fill remaining options into sname and file fields > + * @m:=09=09=09Msg whose sname/file fields may be used for overflow > + * @has_bootfile:=09If true, reserve the file field for the boot file na= me > + * > + * Try the smaller sname field first: small options go there, leaving > + * the larger file field available for big options. When @has_bootfile > + * is set, the file field is reserved for the boot file name per > + * RFC 2132, Section 9.5 and is not used for option overflow. > + * > + * Return: option 52 overload value > + */ > +static enum dhcp_overload fill_overflow(struct msg *m, bool has_bootfile= ) > +{ > +=09enum dhcp_overload overload =3D DHCP_OVERLOAD_NONE; > +=09int sname_off =3D 0, file_off =3D 0; > +=09int o; > + > +=09for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { > +=09=09if (opts[o].slen =3D=3D -1 || opts[o].sent) > +=09=09=09continue; > +=09=09fill_one(m->sname, sizeof(m->sname) - 1, o, &sname_off); > +=09} > + > +=09if (!has_bootfile) { > +=09=09for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { > +=09=09=09if (opts[o].slen =3D=3D -1 || opts[o].sent) > +=09=09=09=09continue; > +=09=09=09if (fill_one(m->file, sizeof(m->file) - 1, o, > +=09=09=09=09 &file_off)) > +=09=09=09=09debug("DHCP: skipping option %i" > +=09=09=09=09 " (overload full)", o); > +=09=09} > +=09} else { > +=09=09for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { > +=09=09=09if (opts[o].slen =3D=3D -1 || opts[o].sent) > +=09=09=09=09continue; > +=09=09=09debug("DHCP: skipping option %i (overload full)", o); > +=09=09} > +=09} You could merge these two: =09for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { =09=09if (opts[o].slen =3D=3D -1 || opts[o].sent) =09=09=09continue; =09=09if (!has_bootfile || =09=09 fill_one(m->file, sizeof(m->file) - 1, o, &file_off)) =09=09=09debug("DHCP: skipping option %i (overload full)", o); =09} or, maybe a bit silly but probably more readable: =09size_t bootfile_size =3D has_bootfile ? sizeof(m->file) - 1 : 0; =09[...] =09for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { =09=09if (fill_one(m->file, bootfile_size, o, &file_off)) =09=09=09debug("DHCP: skipping option %i (overload full)", o); =09} =09=09=09=09=09 > + > +=09if (sname_off) { > +=09=09m->sname[sname_off] =3D 255; > +=09=09overload |=3D DHCP_OVERLOAD_SNAME; > +=09} > + > +=09if (file_off) { > +=09=09m->file[file_off] =3D 255; > +=09=09overload |=3D DHCP_OVERLOAD_FILE; > +=09} > + > +=09return overload; > +} > + > +/** > + * fill() - Fill options in message, with overload into file/sname if ne= eded > + * @m:=09=09=09Message to fill > + * @overload:=09=09Set to option 52 value (0 if none, 1/2/3 per RFC 2132= ) > + * @has_bootfile:=09Reserve 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_bo= otfile) > { > +=09/* Reserve 3 bytes for option 52 (overload) if needed */ > +=09size_t size =3D OPT_MAX - 3; > =09int i, o, offset =3D 0; > =20 > =09for (o =3D 0; o < 255; o++) > @@ -178,17 +243,25 @@ static int fill(struct msg *m) > =09 * Put it there explicitly, unless requested via option 55. > =09 */ > =09if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) > -=09=09fill_one(m->o, OPT_MAX, 53, &offset); > +=09=09fill_one(m->o, size, 53, &offset); > =20 > =09for (i =3D 0; i < opts[55].clen; i++) { > =09=09o =3D opts[55].c[i]; > =09=09if (opts[o].slen !=3D -1) > -=09=09=09fill_one(m->o, OPT_MAX, o, &offset); > +=09=09=09fill_one(m->o, size, o, &offset); > =09} > =20 > =09for (o =3D 0; o < 255; o++) { > =09=09if (opts[o].slen !=3D -1 && !opts[o].sent) > -=09=09=09fill_one(m->o, OPT_MAX, o, &offset); > +=09=09=09fill_one(m->o, size, o, &offset); > +=09} > + > +=09*overload =3D fill_overflow(m, has_bootfile); > + > +=09if (*overload) { > +=09=09m->o[offset++] =3D 52; > +=09=09m->o[offset++] =3D 1; > +=09=09m->o[offset++] =3D *overload; > =09} > =20 > =09m->o[offset++] =3D 255; > @@ -301,6 +374,7 @@ static void opt_set_dns_search(const struct ctx *c, s= ize_t max_len) > int dhcp(const struct ctx *c, struct iov_tail *data) > { > =09char macstr[ETH_ADDRSTRLEN]; > +=09enum dhcp_overload overload; > =09size_t mlen, dlen, opt_len; > =09struct in_addr mask, dst; > =09struct ethhdr eh_storage; > @@ -309,9 +383,12 @@ int dhcp(const struct ctx *c, struct iov_tail *data) > =09const struct ethhdr *eh; > =09const struct iphdr *iph; > =09const struct udphdr *uh; > +=09uint8_t bootfile[128]; > =09struct msg m_storage; > =09struct msg const *m; > +=09bool has_bootfile; > =09struct msg reply; > +=09int bootfile_len; > =09unsigned int i; > =20 > =09eh =3D IOV_REMOVE_HEADER(data, eh_storage); > @@ -462,9 +539,24 @@ int dhcp(const struct ctx *c, struct iov_tail *data) > =09} > =20 > =09if (!c->no_dhcp_dns_search) > -=09=09opt_set_dns_search(c, sizeof(m->o)); > +=09=09opt_set_dns_search(c, OPT_MAX - 3); Why - 3? A comment would be nice to have. > + > +=09/* RFC 2132, Section 9.5: put boot file name in the 'file' header > +=09 * field. Suppress option 67 from the options area and reserve > +=09 * the file field from overload. > +=09 */ > +=09has_bootfile =3D opts[67].slen > 0 && > +=09=09 (size_t)opts[67].slen < sizeof(reply.file); > +=09if (has_bootfile) { > +=09=09memcpy(bootfile, opts[67].s, opts[67].slen); > +=09=09bootfile_len =3D opts[67].slen; > +=09=09opts[67].slen =3D -1; > +=09} > + > +=09dlen =3D offsetof(struct msg, o) + fill(&reply, &overload, has_bootfi= le); > =20 > -=09dlen =3D offsetof(struct msg, o) + fill(&reply); > +=09if (has_bootfile) > +=09=09memcpy(reply.file, bootfile, bootfile_len); > =20 > =09if (m->flags & FLAG_BROADCAST) > =09=09dst =3D in4addr_broadcast; Except for pending comments from David on 3/4 and 4/4, the rest looks good to me. --=20 Stefano