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=dxjMxQ2D; 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 C6B4B5A0265 for ; Fri, 12 Jun 2026 01:05:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781219099; 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=J3RdrHdcdkiJCmiww5K1PF6oQ2TL9fMcVew66GiaZTA=; b=dxjMxQ2DIBlYWs+1dGkbuO4IpuOvWtPfM8J3KwBackPZvPvHuaNI9T2Eue7vTSEzG4v2VN H93daiTKltAyxeBLEaC92isfNmxGa8MnCPWVPk4S4Bwkmg0BFIN6D+6OhSCsm4Iljgl2cc vnnxi1GoguR+CwQPDr1/fPKuyGPrbXY= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-632-4Cp-E1LoNeqrb_KG5pc0pg-1; Thu, 11 Jun 2026 19:04:58 -0400 X-MC-Unique: 4Cp-E1LoNeqrb_KG5pc0pg-1 X-Mimecast-MFC-AGG-ID: 4Cp-E1LoNeqrb_KG5pc0pg_1781219097 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-490b37e1ebfso1343955e9.0 for ; Thu, 11 Jun 2026 16:04:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781219097; x=1781823897; 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=V1Pf2DmMmofWQ8lzggswVtHWj5F3PRpiZZHEa3mQf+Y=; b=Ve3N87f8SckWrWjg0vzRcJr/f3nQuW/thCkESWf24cfxD1a257d4h1gZPQzOOpsxfU D314B4w+4A9d4vgAw1vBjM/azt/CDcSOh32JHtFzjA9piT/KTuOdc1PeKJwia9yKOrbN ofqw2BD2KyY9ai1wB7vS6yobiPUwYNNu81ahLmH/TQh0e/bpI+vsf4AU86tQOvFnwEG8 hZhjSI3rHvFlClGmWxKlX64UqRaqUUKaagYKr1I8rfttQALLICjtqFLD+c6obtBJj+jt iHRVzxIfUcvO3bWiFjwZ+BI/DoXkqHeDnxUzBeJIZB27AkrdrrE4WY/b7MYFHJEYpf8Q T5VA== X-Gm-Message-State: AOJu0YwAZHIZCCdSsgGu7He+vzZT1mPisY3LPTqFVJ9MiQfwGBFmsDNX vaPK9ncjq+18Th0LGiUe+sSbAte7wPjKR/AIO+SbacJa3QzgIIIp0A3HVSpLhH6EXjvidn2sY/s LMd+hXQSMPRwOwT4GLZS02oZK+RKTZIKLA9rnydjnbNpbEurPBofC74hWTsUZeQ== X-Gm-Gg: Acq92OE2u1bHsgDwhVXC2hCy1RDOmdHlQ6cXd4Vd/s3qRgvlO+nTeFzgX+l00+IBmgh iM0A0Uwb/kmM5AFz+EsSQqQUbaZ581gy4TWcAWBhWMvHX+rWn5bZodh47IxMZz1R5nDmBeIPXzY +rmQrckzP5zPe7Njl84tZoRZtANeRmjEZUhcqZsN2GmPgysFgNgiX7CoVMQYSgNJqaySSW+djyv DrRYgJ0L5WCtLk6epmIR5mC9YRhrgfV+hnjykeatptvNRi499aB92JTIq263PXSN0PpFheCUnvC lquAtMHrjX485EfhK/mmfmDpF1orHw6sqFmid88SWJUeOY2T3dpglCvx2te6GtcIWJgEIbprXyz Z47xNDHmGKQuobNaytdrrEEjw70vtnaJ5 X-Received: by 2002:a05:600c:e556:20b0:490:bde3:d112 with SMTP id 5b1f17b1804b1-490ec5088b6mr307785e9.30.1781219096679; Thu, 11 Jun 2026 16:04:56 -0700 (PDT) X-Received: by 2002:a05:600c:e556:20b0:490:bde3:d112 with SMTP id 5b1f17b1804b1-490ec5088b6mr307565e9.30.1781219096148; Thu, 11 Jun 2026 16:04:56 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-490ea95c512sm8558025e9.2.2026.06.11.16.04.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Jun 2026 16:04:55 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH v3 5/6] dhcp: Add option overload Message-ID: <20260612010454.50b8acec@elisabeth> In-Reply-To: <20260601073758.1571317-6-anskuma@redhat.com> References: <20260601073758.1571317-1-anskuma@redhat.com> <20260601073758.1571317-6-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: Fri, 12 Jun 2026 01:04:55 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 3mys5xADIuoMlK2aFmS5uDiG-6sOtALezqRjh2kHQgg_1781219097 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: VSAFZTXWATR2MKACHKSMRGHK6OG75LAJ X-Message-ID-Hash: VSAFZTXWATR2MKACHKSMRGHK6OG75LAJ 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, 1 Jun 2026 13:07:55 +0530 Anshu Kumari wrote: > A user can enter lots of options in command-line which may not fit in > existing buffer, So when the options field is full, overflow remaining > DHCP options into the file and sname fields per RFC 2132 option 52. As David already pointed out, this hides the fact that this patch actually implements the insertion of options in the actual DHCP messages. This makes it pretty hard / impossible to find things later if we need to investigate or fix some issue. Would it be doable to have a separate change just implementing option overload? If not, I think it would be preferable to merge this together with the current 3/6. > Also, when the file field is not used for overload, copy the boot > file URL there directly for legacy PXE clients. >=20 > Link: https://bugs.passt.top/show_bug.cgi?id=3D192 > Signed-off-by: Anshu Kumari > --- > 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 | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 81 insertions(+), 10 deletions(-) >=20 > diff --git a/dhcp.c b/dhcp.c > index 5c6a492..a9d56fd 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -372,14 +372,64 @@ static bool fill_one(uint8_t *buf, size_t size, int= o, int *offset) > =09return false; > } > =20 > +/* RFC 2132, Section 9.3 - Option Overload*/ Nit: missing whitespace before */ > +#define DHCP_OVERLOAD_FILE=091 > +#define DHCP_OVERLOAD_SNAME=092 > + > /** > - * fill() - Fill options in message > + * fill_overflow() - Fill remaining options into sname and file fields > + * @m:=09=09Message whose sname/file fields may be used for overflow > + * > + * Try the smaller sname field first: small options go there, leaving > + * the larger file field available for big options and for the boot > + * file name (option 67) if set. > + * > + * Return: option 52 overload value: 0 if no overflow, > + * DHCP_OVERLOAD_FILE for file, DHCP_OVERLOAD_SNAME for sname, > + * or both OR'd together > + */ > +static int fill_overflow(struct msg *m) > +{ > +=09int sname_off =3D 0, file_off =3D 0, overload =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} > + > +=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 (fill_one(m->file, sizeof(m->file) - 1, o, &file_off)) > +=09=09=09debug("DHCP: skipping option %i (overload full)", o); > +=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=09Message to fill > + * @overload:=09Set to option 52 value (0 if none, 1/2/3 per RFC 2132) Sounds like an enum to me. > * > * Return: current size of options field > */ > -static int fill(struct msg *m) > +static int fill(struct msg *m, int *overload) > { > +=09/* Reserve 3 bytes for option 52 (overload) if needed */ > +=09size_t size =3D OPT_MAX - 3; This isn't ideal in the sense that if we have, at the end of the options, an option that's three or less bytes in total (one or zero bytes of value), and we are at OPT_MAX - 3, we could encode it without overload. I don't see a real issue in keeping this for the sake of simplicity, especially because that's such an unlikely corner case, and the consequences of this non-ideal implementation are really minor if any, but maybe (I haven't really thought it through) to always keep three bytes free, and then add another loop checking if we have a single option that's three bytes long or less? > =09int i, o, offset =3D 0; > =20 > =09for (o =3D 0; o < 255; o++) > @@ -390,20 +440,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=09if (fill_one(m->o, OPT_MAX, 53, &offset)) > -=09=09=09 debug("DHCP: skipping option 53"); > +=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=09if (fill_one(m->o, OPT_MAX, o, &offset)) > -=09=09=09=09debug("DHCP: skipping option %i", o); > +=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=09if (fill_one(m->o, OPT_MAX, o, &offset)) > -=09=09=09=09debug("DHCP: skipping option %i", o); > +=09=09=09fill_one(m->o, size, o, &offset); > +=09} > + > +=09*overload =3D fill_overflow(m); > + > +=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; > @@ -528,6 +583,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data) > =09struct msg const *m; > =09struct msg reply; > =09unsigned int i; > +=09int overload; > =20 > =09eh =3D IOV_REMOVE_HEADER(data, eh_storage); > =09iph =3D IOV_PEEK_HEADER(data, iph_storage); > @@ -677,9 +733,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); > + > +=09for (i =3D 0; i < (unsigned int)c->custom_opts_count; i++) { > +=09=09uint8_t code =3D c->custom_opts[i].code; > + > +=09=09opts[code].slen =3D c->custom_opts[i].len; > +=09=09memcpy(opts[code].s, c->custom_opts[i].val, > +=09=09 c->custom_opts[i].len); > +=09} > + > +=09dlen =3D offsetof(struct msg, o) + fill(&reply, &overload); > =20 > -=09dlen =3D offsetof(struct msg, o) + fill(&reply); > +=09/* Copy boot file name into the file field for legacy PXE clients, > +=09 * unless the file field is already used for option overload. > +=09 */ But RFC 2132, section 9.5, says: This option is used to identify a bootfile when the 'file' field in the DHCP header has been used for DHCP options. and I think we should interpret that "when" as "if and only if" by default (that's the least surprising way from a linguistic perspective), so, as a consequence, we should never duplicate that in the replies we send. That is, if we can insert it in 'file', we should do that, and we should use option 67 _only if_ we can't. Not always. Not if it's already in 'file'. Now, we don't know if we need option overload until we're done inserting options,=20 > +=09if (!(overload & DHCP_OVERLOAD_FILE) && > +=09 opts[67].slen > 0 && (size_t)opts[67].slen < sizeof(reply.file)) > +=09=09memcpy(reply.file, opts[67].s, opts[67].slen); > =20 > =09if (m->flags & FLAG_BROADCAST) > =09=09dst =3D in4addr_broadcast; --=20 Stefano