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=202508 header.b=r1J82EbT; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id D433D5A027B for ; Wed, 13 Aug 2025 04:29:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1755052167; bh=Hjr5hz4xETyem2fTKkI1pj4HqgIL5xX6uBDnPs23jmY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=r1J82EbTHUWQZMvj1rI+1nWrtcQUsotRuF65y2kk9ddUJhrOHQFS3Ul9hW0nT2vdp EXB8owLW7FTOc9dtJ/q9B234F+i2YgIr2foaFFXk+DaFSCB76OJJR1pVvirvI5DutF 9JuE1vkYYfOgYEn1f0AEjw91RLsJMtq6RhubTRYJO31SV3nkC6NZKNbqV/jc7RD/YA M0L3Gc66Yr6JMjQ0NXfexiDl6SWzcYUd2Ebw0FMTavyUtFwMsc2uk5vWmThYAk+P1F uIPZmIXFng23NS0OEmkwp+ScjZKh0MIkpG2KBUB+yEHzUkCKiaoMLiFzAEdSQu4Sn1 iWHK1ibq02ttQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4c1slC5826z4xQV; Wed, 13 Aug 2025 12:29:27 +1000 (AEST) Date: Wed, 13 Aug 2025 12:27:52 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v8 17/30] dhcp: Convert to iov_tail Message-ID: References: <20250805154628.301343-1-lvivier@redhat.com> <20250805154628.301343-18-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="sytvoOy4WMiTaVDO" Content-Disposition: inline In-Reply-To: Message-ID-Hash: AVJECAOJ27YSEE4KQGUV53IUA54IVOH7 X-Message-ID-Hash: AVJECAOJ27YSEE4KQGUV53IUA54IVOH7 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: --sytvoOy4WMiTaVDO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 08, 2025 at 11:33:24AM +0200, Laurent Vivier wrote: > On 06/08/2025 06:38, David Gibson wrote: > > On Tue, Aug 05, 2025 at 05:46:15PM +0200, Laurent Vivier wrote: > > > Use packet_data() and extract headers using IOV_REMOVE_HEADER() > > > and IOV_PEEK_HEADER() rather than packet_get(). > >=20 > > Unlike the previous patch, I think using iov_tail does work here, > > because there's a single scan through the options, rather than > > repeatedly scanning for options of specific types. > >=20 > > > Signed-off-by: Laurent Vivier > > > --- > > > dhcp.c | 46 ++++++++++++++++++++++++++++------------------ > > > 1 file changed, 28 insertions(+), 18 deletions(-) > > >=20 > > > diff --git a/dhcp.c b/dhcp.c > > > index b0de04be6f27..cf73d4b07767 100644 > > > --- a/dhcp.c > > > +++ b/dhcp.c > > > @@ -302,27 +302,33 @@ static void opt_set_dns_search(const struct ctx= *c, size_t max_len) > > > */ > > > int dhcp(const struct ctx *c, const struct pool *p) > > > { > > > - size_t mlen, dlen, offset =3D 0, opt_len, opt_off =3D 0; > > > char macstr[ETH_ADDRSTRLEN]; > > > + size_t mlen, dlen, opt_len; > > > struct in_addr mask, dst; > > > + struct ethhdr eh_storage; > > > + struct iphdr iph_storage; > > > + struct udphdr uh_storage; > > > const struct ethhdr *eh; > > > const struct iphdr *iph; > > > const struct udphdr *uh; > > > + struct iov_tail data; > > > struct msg const *m; > >=20 > > Pre-existing, but I'm a bit baffled as to what the (const *) is doing h= ere. > >=20 > > > struct msg reply; > > > unsigned int i; > > > + struct msg m_storage; > > > - eh =3D packet_get(p, 0, offset, sizeof(*eh), NULL); > > > - offset +=3D sizeof(*eh); > > > + if (!packet_data(p, 0, &data)) > > > + return -1; > > > - iph =3D packet_get(p, 0, offset, sizeof(*iph), NULL); > > > + eh =3D IOV_REMOVE_HEADER(&data, eh_storage); > > > + iph =3D IOV_PEEK_HEADER(&data, iph_storage); > > > if (!eh || !iph) > > > return -1; > > > - offset +=3D iph->ihl * 4UL; > > > - uh =3D packet_get(p, 0, offset, sizeof(*uh), &mlen); > > > - offset +=3D sizeof(*uh); > > > + if (!iov_tail_drop(&data, iph->ihl * 4UL)) > > > + return -1; > > > + uh =3D IOV_REMOVE_HEADER(&data, uh_storage); > > > if (!uh) > > > return -1; > > > @@ -332,7 +338,10 @@ int dhcp(const struct ctx *c, const struct pool = *p) > > > if (c->no_dhcp) > > > return 1; > > > - m =3D packet_get(p, 0, offset, offsetof(struct msg, o), &opt_len); > > > + mlen =3D iov_tail_size(&data); > > > + m =3D (struct msg const *)iov_remove_header_(&data, &m_storage, > > > + offsetof(struct msg, o), > > > + __alignof__(struct msg)); > > > if (!m || > > > mlen !=3D ntohs(uh->len) - sizeof(*uh) || > > > mlen < offsetof(struct msg, o) || > > > @@ -355,27 +364,28 @@ int dhcp(const struct ctx *c, const struct pool= *p) > > > memset(&reply.file, 0, sizeof(reply.file)); > > > reply.magic =3D m->magic; > > > - offset +=3D offsetof(struct msg, o); > > > - > > > for (i =3D 0; i < ARRAY_SIZE(opts); i++) > > > opts[i].clen =3D -1; > > > - while (opt_off + 2 < opt_len) { > > > - const uint8_t *olen, *val; > > > + opt_len =3D iov_tail_size(&data); > > > + while (opt_len >=3D 2) { > > > + uint8_t olen_storage, type_storage; > > > + const uint8_t *olen; > > > uint8_t *type; > > > - type =3D packet_get(p, 0, offset + opt_off, 1, NULL); > > > - olen =3D packet_get(p, 0, offset + opt_off + 1, 1, NULL); > > > + type =3D IOV_REMOVE_HEADER(&data, type_storage); > > > + olen =3D IOV_REMOVE_HEADER(&data, olen_storage); > >=20 > > It seems a bit mad to access single bytes via 8-byte pointers, but > > it's probably not worth the hassle of handling it differently in this > > one case. > >=20 > > > if (!type || !olen) > > > return -1; > > > - val =3D packet_get(p, 0, offset + opt_off + 2, *olen, NULL); > > > - if (!val) > > > + opt_len =3D iov_tail_size(&data); > > > + if (opt_len < *olen) > > > return -1; > > > - memcpy(&opts[*type].c, val, *olen); > > > + iov_to_buf(&data.iov[0], data.cnt, data.off, &opts[*type].c, *olen= ); > >=20 > > So, IIUC, if *olen is much too big, this is still safe.. > >=20 > > > opts[*type].clen =3D *olen; > >=20 > > .. but recording *olen unedited as the length of the option is > > probably wrong in that case. >=20 > I don't understand how to edit *olen. There is no change regarding the or= iginal code. Sorry, I was concerned about a malformed packet which gave a long olen when there isn't actually that much that. I missed that you tested for (opt_len < *olen) above, which handles that case. > >=20 > > > - opt_off +=3D *olen + 2; > > > + iov_tail_drop(&data, *olen); > > > + opt_len -=3D *olen; > >=20 > > Isn't the stanza above doing the equivalent of an > > iov_remove_header_()? >=20 > No, in fact iov_remove_header_() copy to the buffer only if the data are > discontinuous, in this case we want to copy the data unconditionally to e= dit > them later. We don't want to edit the data in the iovec buffer. Ah, right. Unconditionally linearizing / copying a header seems like it might be a useful thing in more places. I wonder if we should make that its own helper and we can use it both here and in the slow path of iov_remove_header_(). --=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 --sytvoOy4WMiTaVDO Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmib+CcACgkQzQJF27ox 2Gf5nw/9Gyn1Jz33e4QhMCDv9aJugUR6qbqgslcr2yJBD0XNPX99fqpIF/58x20K atL8Abtv7t9S6h3XhXLfklO9fO+HXTUtZs7ct0tQ11p52KpDwKRlpL1Ni5qqVI0X e5DrfZcqubj1yhMw2hNPFc/VhBY25nHt0vpBwUGTSqiB75GK2GeV/LAOd4nCu8ZM cx6CrdD9BtJ4JajflyFa0x8glrdP6x2iSq9DjgW5M2aDjwS06q4PeqV8Q2Pi2cVO S01X9SEKhhjDYyjOMuK6BDg7Pm9N8EKNkW2WTPTCcCmUVSsCluIXXNvNIK0K+y30 IQpcU8r/usB/Vi5w9Pu5nF4h1+SkCdOcfPAs2xoLStCBI7s/IeHJEL6yW25prEjt XE6H9DnCsJzOjIanZLfEI9RcnCmllcCFwWZLgw7OwLrm7qujy8WREQFvdJ0AT+GQ 3W2TFuIe3LXvzVZ7TD9hT2IiOq/OQ2vuJFkJBlRFxLOBfNKJMImA4tmADCGAfzDR /CnRnE2qyyiSzDiqsvOKHm5voWA8V0mFZqZtw5CpYGZQwJ1aCbnqxLQuaEgiKBeG dlPP03f65+uhiVkAtVZN25DMPk7+44iEIXzoQJbgJaV07XcOIYVna1+2a9/ZLf29 nOE4rgEjWyR4ANreBG8I16dMZJSsSRQ1TWo74a2ryd/J/VMoIlE= =5ySq -----END PGP SIGNATURE----- --sytvoOy4WMiTaVDO--