From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202410 header.b=Yh/bCHgf; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id DC1775A004C for ; Mon, 25 Nov 2024 11:16:35 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202410; t=1732529781; bh=v88Gp7aSPHezyCLcm08MEZDC0nBvVrkN4/pvuBlmXDw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Yh/bCHgfGwm4t1KFpzsFscMAvLf/1E+ZwYkRQC7mAoxJM2dOfijnxj6P7A29UAyQb 8wrtBZ3+B+jr823ARMFA4zys73hLp0DaS9FsgBMLYFrjuaeHWn+HhrDFzeFOJRqvcq WjnPM5PDyNUiHqSEskKMy3FbdgNk6kemZNvBEtqYoV1lyWT758d45eMw/zKEaMFolD ogQmsUzumyeEN/AqMIfCQVBlHlr3mvfA74lQzrWuQ2Ric4/NMQew45NeGnh7wA3Rux 7p6SbjdsGYamA8EnnDGUS1IpZUd9rtNYunXdf5uK1/gc8C07AvVFeomSQuknSUOsX7 Y5sItyWMpMjCA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4XxhSP5VCMz4xdL; Mon, 25 Nov 2024 21:16:21 +1100 (AEDT) Date: Mon, 25 Nov 2024 20:18:36 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 1/3] dhcp: Use -1 as "missing option" length instead of 0 Message-ID: References: <20241125000423.4131458-1-sbrivio@redhat.com> <20241125000423.4131458-2-sbrivio@redhat.com> <20241125093010.362f1aa8@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qvQ2JqJsNPp1Yesm" Content-Disposition: inline In-Reply-To: <20241125093010.362f1aa8@elisabeth> Message-ID-Hash: JJ27H644W3NHFCMEI7DWFJ64BMDI4BJC X-Message-ID-Hash: JJ27H644W3NHFCMEI7DWFJ64BMDI4BJC X-MailFrom: dgibson@gandalf.ozlabs.org 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 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: --qvQ2JqJsNPp1Yesm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 25, 2024 at 09:30:10AM +0100, Stefano Brivio wrote: > On Mon, 25 Nov 2024 12:08:35 +1100 > David Gibson wrote: >=20 > > On Mon, Nov 25, 2024 at 01:04:21AM +0100, Stefano Brivio wrote: > > > We want to add support for option 80 (Rapid Commit, RFC 4039), whose > > > length is 0. > > >=20 > > > Signed-off-by: Stefano Brivio > > > --- > > > dhcp.c | 27 ++++++++++++++++++++------- > > > 1 file changed, 20 insertions(+), 7 deletions(-) > > >=20 > > > diff --git a/dhcp.c b/dhcp.c > > > index a06f143..2fe4a4d 100644 > > > --- a/dhcp.c > > > +++ b/dhcp.c > > > @@ -36,9 +36,9 @@ > > > /** > > > * struct opt - DHCP option > > > * @sent: Convenience flag, set while filling replies > > > - * @slen: Length of option defined for server > > > + * @slen: Length of option defined for server, -1 if not going to be= sent > > > * @s: Option payload from server > > > - * @clen: Length of option received from client > > > + * @clen: Length of option received from client, -1 if not received > > > * @c: Option payload from client > > > */ > > > struct opt { > > > @@ -154,17 +154,17 @@ static int fill(struct msg *m) > > > * option 53 at the beginning of the list. > > > * Put it there explicitly, unless requested via option 55. > > > */ > > > - if (!memchr(opts[55].c, 53, opts[55].clen)) > > > + if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) > > > fill_one(m, 53, &offset); > > > =20 > > > for (i =3D 0; i < opts[55].clen; i++) { > > > o =3D opts[55].c[i]; > > > - if (opts[o].slen) > > > + if (opts[o].slen !=3D -1) > > > fill_one(m, o, &offset); > > > } > > > =20 > > > for (o =3D 0; o < 255; o++) { > > > - if (opts[o].slen && !opts[o].sent) > > > + if (opts[o].slen !=3D -1 && !opts[o].sent) > > > fill_one(m, o, &offset); > > > } > > > =20 > > > @@ -264,6 +264,9 @@ static void opt_set_dns_search(const struct ctx *= c, size_t max_len) > > > ".\xc0"); > > > } > > > } > > > + > > > + if (!opts[119].slen) > > > + opts[119].slen =3D -1; > > > } > > > =20 > > > /** > > > @@ -313,6 +316,13 @@ int dhcp(const struct ctx *c, const struct pool = *p) > > > =20 > > > offset +=3D offsetof(struct msg, o); > > > =20 > > > + for (i =3D 0; i < ARRAY_SIZE(opts); i++) { > > > + if (!opts[i].slen) > > > + opts[i].slen =3D -1; > > > + > > > + opts[i].clen =3D -1; > > > + } =20 > >=20 > > Could this move to dhcp_init()? I think there you wouldn't need test > > and could unconditionally initialize all the lengths to -1 before > > initializing the options we actually use. >=20 > No, because dhcp_init() is run only once, and 'opts' at this point > represents the status from the previous run, so: >=20 > - we need to unconditionally reset all the 'clen' attributes which were > set in the previous run Ah, yes of course. > - we need to reset the 'slen' attributes for zero-length options (it's > just option 80 at the moment) because we need to re-evaluate their > inclusion. Sure, I could also clean things up at the end of any run, > but this is more practical and robust Hrm... I see that you need to do _something_ here, but the zero length check still seems weird to me. Ideally, this change would mean that "no option" is always represented by -1 - but in a few places 0 sort of still does as well. There's no nice way in C to statically initialise all the entries to -1, so I can see why we have to set things to -1 with code in dhcp_init(), but I don't much like having ambiguous representation past that point. Shouldn't whether we reset a server side option be dependent only on which option it is, not whether it was zero-length the last time? The fact that this only affects option 80, which happens to be zero-length seems only correct by accident. >=20 > > > while (opt_off + 2 < opt_len) { > > > const uint8_t *olen, *val; > > > uint8_t *type; > > > @@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *= p) > > > if (opts[53].c[0] =3D=3D DHCPDISCOVER) { > > > info("DHCP: offer to discover"); > > > opts[53].s[0] =3D DHCPOFFER; > > > - } else if (opts[53].c[0] =3D=3D DHCPREQUEST || !opts[53].clen) { > > > - info("%s: ack to request", opts[53].clen ? "DHCP" : "BOOTP"); > > > + } else if (opts[53].c[0] =3D=3D DHCPREQUEST || opts[53].clen <=3D 0= ) { > > > + info("%s: ack to request", > > > + (opts[53].clen <=3D 0) ? "DHCP" : "BOOTP"); =20 > >=20 > > Should this be <=3D 0, or < 0? i.e. Wouldn't even an empty option 53 > > indicate we're dealing with DHCP rather than BOOTP? >=20 > It should really be <=3D 0, preserving the existing behaviour, because if > option 53 is empty, we don't know what kind of DHCP message that is. We > know for sure that it's not a valid DHCP message, but it probably is a > valid BOOTP message (with a vendor extension). Ah, that makes sense. A comment to that effect might be useful. > This might look like speculation, but there are some half-DHCP > implementations from the 1990s which we can happily handle as BOOTP > clients, but not really as DHCP. After all the fun we had with wattcp32 > and mTCP I would say it's not unlikely. >=20 > > > opts[53].s[0] =3D DHCPACK; > > > } else { > > > return -1; > > > @@ -374,6 +385,8 @@ int dhcp(const struct ctx *c, const struct pool *= p) > > > ((struct in_addr *)opts[6].s)[i] =3D c->ip4.dns[i]; > > > opts[6].slen +=3D sizeof(uint32_t); > > > } > > > + if (!opts[6].slen) > > > + opts[6].slen =3D -1; > > > =20 > > > if (!c->no_dhcp_dns_search) > > > opt_set_dns_search(c, sizeof(m->o)); =20 >=20 --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --qvQ2JqJsNPp1Yesm Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmdEQOcACgkQzQJF27ox 2GcB+w//Vwg3/fTDnZ33Gxv327TOclS8UFzAiLZHLbWPHIvw3LZNpRFTGjqtnnbK dsQ8xu+qWv12xOGjql/gxc1UzBb2g4Aqnv3MkIFMmd3ycHh4U1GEZ9I5XmPdmQ7Q rI3AKg+GGBayrwaQRt42OIpMI3RNSyE+CxP//T3c1TBNW4Rfn8YWcR61hy+HWsql qCnU64JfM6k0b6YIs639lZLHQkOA9uINjcijcbbXsShUjexp3LonuR/9UwMgEtew uwEztzN0BJ+T1G3qqa3AmV7bwqj7QdFzAQ7/Wzkjz+V4v7hB8OdZDpUGKt90nA8K xupGWmit2oh/HNCBi+9zI4rRad2IhGcD1X/F1xDqtK61AeJD8daBYfVfvTu8Ze1l TRoI2v3nUzXSbkPkbbYg5y5wuFA96lAhgyS4H0i12+dga3Gc3UL9CED88DEy2Ve+ Uz+/lxdvS2/8ZLG/64l2hHID7Rz94ih76Kn+83M+7nhP+p0Tu2RaEmKD5mfkt2Yn 7I8Ez4s7UzDyoq1yqYDmVrC+VbLcrzxvRZ3NCCH3/bSlGXtfbTviDlB3QyxWioPV skPNXY+z18ldFPcl02EUcIYcp8Vuf/K25C+vUy2n5A7UP14V4vkb4+b7CD1I9jNH 19TZjdvP1+OSLwm9cfWqsuYDNYLZM5W6xSgM7giWdR69yC459Ug= =/1ok -----END PGP SIGNATURE----- --qvQ2JqJsNPp1Yesm--