From: David Gibson <david@gibson.dropbear.id.au>
To: Laurent Vivier <lvivier@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v8 16/30] dhcpv6: Use iov_tail in dhcpv6_opt()
Date: Wed, 6 Aug 2025 14:14:06 +1000 [thread overview]
Message-ID: <aJLWjhQ2YhEkKZvi@zatzit> (raw)
In-Reply-To: <20250805154628.301343-17-lvivier@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 13085 bytes --]
On Tue, Aug 05, 2025 at 05:46:14PM +0200, Laurent Vivier wrote:
> dhcpv6_opt() and its callers are refactored for iov_tail option parsing,
> replacing direct offset management for improved robustness.
>
> Its signature is now `bool dhcpv6_opt(iov_tail *data, type)`. `*data` (in/out)
> points to a found option on `true` return or is restored on `false`.
> The main dhcpv6() function uses IOV_REMOVE_HEADER for the msg_hdr, then
> passes the iov_tail (now at options start) to the new dhcpv6_opt().
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Hmm. I'm not sure this is a great use case for iov_tail - the code is
repeatedly scanning the same options, so there's a whole bunch of
rewinding. It works, but it seems awkward.
DHCP is a slow path, anyway, so maybe we'd be better off just
linearizing the entire packet and using plain old pointers to scan
through the options.
> ---
> dhcpv6.c | 179 ++++++++++++++++++++++++++++++++-----------------------
> iov.c | 1 -
> 2 files changed, 104 insertions(+), 76 deletions(-)
>
> diff --git a/dhcpv6.c b/dhcpv6.c
> index ae06e646f92f..e93acaf9955e 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -280,112 +280,125 @@ static struct resp_not_on_link_t {
>
> /**
> * dhcpv6_opt() - Get option from DHCPv6 message
> - * @p: Packet pool, single packet with UDP header
> - * @offset: Offset to look at, 0: end of header, set to option start
> + * @data: Buffer with options, set to matching option on return
> * @type: Option type to look up, network order
> *
> - * Return: pointer to option header, or NULL on malformed or missing option
> + * Return: true if found and @data points to the option header,
> + * or false on malformed or missing option and @data is
> + * unmodified.
> */
> -static struct opt_hdr *dhcpv6_opt(const struct pool *p, size_t *offset,
> - uint16_t type)
> +static bool dhcpv6_opt(struct iov_tail *data, uint16_t type)
> {
> - struct opt_hdr *o;
> - size_t left;
> + struct iov_tail head = *data;
> + struct opt_hdr o_storage;
> + const struct opt_hdr *o;
>
> - ASSERT(*offset >= UDP_MSG_HDR_SIZE);
> -
> - while ((o = packet_get_try(p, 0, *offset, sizeof(*o), &left))) {
> + while ((o = IOV_PEEK_HEADER(data, o_storage))) {
> unsigned int opt_len = ntohs(o->l) + sizeof(*o);
>
> - if (ntohs(o->l) > left)
> - return NULL;
> + if (opt_len > iov_tail_size(data))
> + break;
>
> if (o->t == type)
> - return o;
> + return true;
>
> - *offset += opt_len;
> + iov_tail_drop(data, opt_len);
> }
>
> - return NULL;
> + *data = head;
> + return false;
> }
>
> /**
> * dhcpv6_ia_notonlink() - Check if any IA contains non-appropriate addresses
> - * @p: Packet pool, single packet starting from UDP header
> + * @data: Data to look at, packet starting from UDP header (input/output)
> * @la: Address we want to lease to the client
> *
> - * Return: pointer to non-appropriate IA_NA or IA_TA, if any, NULL otherwise
> + * Return: true and @data points to non-appropriate IA_NA or IA_TA, if any,
> + * false otherwise and @data is unmodified
> */
> -static struct opt_hdr *dhcpv6_ia_notonlink(const struct pool *p,
> - struct in6_addr *la)
> +static bool dhcpv6_ia_notonlink(struct iov_tail *data,
> + struct in6_addr *la)
> {
> int ia_types[2] = { OPT_IA_NA, OPT_IA_TA }, *ia_type;
> + struct opt_ia_addr opt_addr_storage;
> const struct opt_ia_addr *opt_addr;
> + struct iov_tail current, ia_base;
> + struct opt_ia_na ia_storage;
> char buf[INET6_ADDRSTRLEN];
> + const struct opt_ia_na *ia;
> struct in6_addr req_addr;
> + struct opt_hdr h_storage;
> const struct opt_hdr *h;
> - struct opt_hdr *ia;
> - size_t offset;
>
> foreach(ia_type, ia_types) {
> - offset = UDP_MSG_HDR_SIZE;
> - while ((ia = dhcpv6_opt(p, &offset, *ia_type))) {
> - if (ntohs(ia->l) < OPT_VSIZE(ia_na))
> - return NULL;
> -
> - offset += sizeof(struct opt_ia_na);
> + current = *data;
> + while (dhcpv6_opt(¤t, *ia_type)) {
> + ia_base = current;
> + ia = IOV_REMOVE_HEADER(¤t, ia_storage);
> + if (!ia || ntohs(ia->hdr.l) < OPT_VSIZE(ia_na))
> + goto notfound;
> +
> + while (dhcpv6_opt(¤t, OPT_IAAADR)) {
> + h = IOV_PEEK_HEADER(¤t, h_storage);
> + if (!h || ntohs(h->l) != OPT_VSIZE(ia_addr))
> + goto notfound;
> +
> + opt_addr = IOV_REMOVE_HEADER(¤t,
> + opt_addr_storage);
> + if (!opt_addr)
> + goto notfound;
>
> - while ((h = dhcpv6_opt(p, &offset, OPT_IAAADR))) {
> - if (ntohs(h->l) != OPT_VSIZE(ia_addr))
> - return NULL;
> -
> - opt_addr = (const struct opt_ia_addr *)h;
> req_addr = opt_addr->addr;
> if (!IN6_ARE_ADDR_EQUAL(la, &req_addr))
> - goto err;
> -
> - offset += sizeof(struct opt_ia_addr);
> + goto notonlink;
> }
> }
> }
>
> - return NULL;
> +notfound:
> + return false;
>
> -err:
> +notonlink:
> info("DHCPv6: requested address %s not on link",
> inet_ntop(AF_INET6, &req_addr, buf, sizeof(buf)));
> - return ia;
> + *data = ia_base;
> + return true;
> }
>
> /**
> * dhcpv6_send_ia_notonlink() - Send NotOnLink status
> - * @c: Execution context
> - * @ia: Pointer to non-appropriate IA_NA or IA_TA
> - * @client_id: Client ID message option
> - * xid: Transaction ID for message exchange
> + * @c: Execution context
> + * @ia_base: Non-appropriate IA_NA or IA_TA base
> + * @client_id_base: Client ID message option base
> + * @len: Client ID length
> + * @xid: Transaction ID for message exchange
> */
> -static void dhcpv6_send_ia_notonlink(struct ctx *c, struct opt_hdr *ia,
> - const struct opt_hdr *client_id,
> - uint32_t xid)
> +static void dhcpv6_send_ia_notonlink(struct ctx *c,
> + const struct iov_tail *ia_base,
> + const struct iov_tail *client_id_base,
> + int len, uint32_t xid)
> {
> const struct in6_addr *src = &c->ip6.our_tap_ll;
> + struct opt_hdr *ia = (struct opt_hdr *)resp_not_on_link.var;
> size_t n;
>
> info("DHCPv6: received CONFIRM with inappropriate IA,"
> " sending NotOnLink status in REPLY");
>
> - ia->l = htons(OPT_VSIZE(ia_na) + sizeof(sc_not_on_link));
> -
> n = sizeof(struct opt_ia_na);
> - memcpy(resp_not_on_link.var, ia, n);
> + iov_to_buf(&ia_base->iov[0], ia_base->cnt, ia_base->off,
> + resp_not_on_link.var, n);
> + ia->l = htons(OPT_VSIZE(ia_na) + sizeof(sc_not_on_link));
> memcpy(resp_not_on_link.var + n, &sc_not_on_link,
> sizeof(sc_not_on_link));
>
> n += sizeof(sc_not_on_link);
> - memcpy(resp_not_on_link.var + n, client_id,
> - sizeof(struct opt_hdr) + ntohs(client_id->l));
> + iov_to_buf(&client_id_base->iov[0], client_id_base->cnt,
> + client_id_base->off, resp_not_on_link.var + n,
> + sizeof(struct opt_hdr) + len);
>
> - n += sizeof(struct opt_hdr) + ntohs(client_id->l);
> + n += sizeof(struct opt_hdr) + len;
>
> n = offsetof(struct resp_not_on_link_t, var) + n;
>
> @@ -474,17 +487,19 @@ search:
>
> /**
> * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
> + * @data: Data to look at
> * @c: Execution context
> * @buf: Response message buffer where options will be appended
> * @offset: Offset in message buffer for new options
> *
> * Return: updated length of response message buffer.
> */
> -static size_t dhcpv6_client_fqdn_fill(const struct pool *p, const struct ctx *c,
> +static size_t dhcpv6_client_fqdn_fill(const struct iov_tail *data,
> + const struct ctx *c,
> char *buf, int offset)
>
> {
> - struct opt_client_fqdn const *req_opt;
> + struct iov_tail current = *data;
> struct opt_client_fqdn *o;
> size_t opt_len;
>
> @@ -502,14 +517,16 @@ static size_t dhcpv6_client_fqdn_fill(const struct pool *p, const struct ctx *c,
> }
>
> o = (struct opt_client_fqdn *)(buf + offset);
> + o->flags = 0x00;
> encode_domain_name(o->domain_name, c->fqdn);
> - req_opt = (struct opt_client_fqdn *)dhcpv6_opt(p,
> - &(size_t){ UDP_MSG_HDR_SIZE },
> - OPT_CLIENT_FQDN);
> - if (req_opt && req_opt->flags & 0x01 /* S flag */)
> - o->flags = 0x02 /* O flag */;
> - else
> - o->flags = 0x00;
> + if (dhcpv6_opt(¤t, OPT_CLIENT_FQDN)) {
> + struct opt_client_fqdn req_opt_storage;
> + struct opt_client_fqdn const *req_opt;
> +
> + req_opt = IOV_PEEK_HEADER(¤t, req_opt_storage);
> + if (req_opt && req_opt->flags & 0x01 /* S flag */)
> + o->flags = 0x02 /* O flag */;
> + }
>
> opt_len++;
>
> @@ -531,14 +548,18 @@ static size_t dhcpv6_client_fqdn_fill(const struct pool *p, const struct ctx *c,
> int dhcpv6(struct ctx *c, const struct pool *p,
> const struct in6_addr *saddr, const struct in6_addr *daddr)
> {
> - const struct opt_hdr *client_id, *server_id, *ia;
> + const struct opt_server_id *server_id = NULL;
> + struct iov_tail data, opt, client_id_base;
> + const struct opt_hdr *client_id = NULL;
> + struct opt_server_id server_id_storage;
> + const struct opt_ia_na *ia = NULL;
> + struct opt_hdr client_id_storage;
> + struct opt_ia_na ia_storage;
> const struct in6_addr *src;
> struct msg_hdr mh_storage;
> const struct msg_hdr *mh;
> struct udphdr uh_storage;
> const struct udphdr *uh;
> - struct opt_hdr *bad_ia;
> - struct iov_tail data;
> size_t mlen, n;
>
> if (!packet_data(p, 0, &data))
> @@ -565,20 +586,26 @@ int dhcpv6(struct ctx *c, const struct pool *p,
>
> src = &c->ip6.our_tap_ll;
>
> - mh = IOV_PEEK_HEADER(&data, mh_storage);
> + mh = IOV_REMOVE_HEADER(&data, mh_storage);
> if (!mh)
> return -1;
>
> - client_id = dhcpv6_opt(p, &(size_t){ UDP_MSG_HDR_SIZE }, OPT_CLIENTID);
> + client_id_base = data;
> + if (dhcpv6_opt(&client_id_base, OPT_CLIENTID))
> + client_id = IOV_PEEK_HEADER(&client_id_base, client_id_storage);
> if (!client_id || ntohs(client_id->l) > OPT_VSIZE(client_id))
> return -1;
>
> - server_id = dhcpv6_opt(p, &(size_t){ UDP_MSG_HDR_SIZE }, OPT_SERVERID);
> - if (server_id && ntohs(server_id->l) != OPT_VSIZE(server_id))
> + opt = data;
> + if (dhcpv6_opt(&opt, OPT_SERVERID))
> + server_id = IOV_PEEK_HEADER(&opt, server_id_storage);
> + if (server_id && ntohs(server_id->hdr.l) != OPT_VSIZE(server_id))
> return -1;
>
> - ia = dhcpv6_opt(p, &(size_t){ UDP_MSG_HDR_SIZE }, OPT_IA_NA);
> - if (ia && ntohs(ia->l) < MIN(OPT_VSIZE(ia_na), OPT_VSIZE(ia_ta)))
> + opt = data;
> + if (dhcpv6_opt(&opt, OPT_IA_NA))
> + ia = IOV_PEEK_HEADER(&opt, ia_storage);
> + if (ia && ntohs(ia->hdr.l) < MIN(OPT_VSIZE(ia_na), OPT_VSIZE(ia_ta)))
> return -1;
>
> resp.hdr.type = TYPE_REPLY;
> @@ -593,9 +620,10 @@ int dhcpv6(struct ctx *c, const struct pool *p,
> if (mh->type == TYPE_CONFIRM && server_id)
> return -1;
>
> - if ((bad_ia = dhcpv6_ia_notonlink(p, &c->ip6.addr))) {
> + if (dhcpv6_ia_notonlink(&data, &c->ip6.addr)) {
>
> - dhcpv6_send_ia_notonlink(c, bad_ia, client_id, mh->xid);
> + dhcpv6_send_ia_notonlink(c, &data, &client_id_base,
> + ntohs(client_id->l), mh->xid);
>
> return 1;
> }
> @@ -607,7 +635,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
> memcmp(&resp.server_id, server_id, sizeof(resp.server_id)))
> return -1;
>
> - if (ia || dhcpv6_opt(p, &(size_t){ UDP_MSG_HDR_SIZE }, OPT_IA_TA))
> + if (ia || dhcpv6_opt(&data, OPT_IA_TA))
> return -1;
>
> info("DHCPv6: received INFORMATION_REQUEST, sending REPLY");
> @@ -633,13 +661,14 @@ int dhcpv6(struct ctx *c, const struct pool *p,
> if (ia)
> resp.ia_na.iaid = ((struct opt_ia_na *)ia)->iaid;
>
> - memcpy(&resp.client_id, client_id,
> - ntohs(client_id->l) + sizeof(struct opt_hdr));
> + iov_to_buf(&client_id_base.iov[0], client_id_base.cnt,
> + client_id_base.off, &resp.client_id,
> + ntohs(client_id->l) + sizeof(struct opt_hdr));
>
> n = offsetof(struct resp_t, client_id) +
> sizeof(struct opt_hdr) + ntohs(client_id->l);
> n = dhcpv6_dns_fill(c, (char *)&resp, n);
> - n = dhcpv6_client_fqdn_fill(p, c, (char *)&resp, n);
> + n = dhcpv6_client_fqdn_fill(&data, c, (char *)&resp, n);
>
> resp.hdr.xid = mh->xid;
>
> diff --git a/iov.c b/iov.c
> index f519eb3cfeaf..d17d4dd3da09 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -109,7 +109,6 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
> *
> * Return: the number of bytes successfully copied.
> */
> -/* cppcheck-suppress [staticFunction] */
> size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
> size_t offset, void *buf, size_t bytes)
> {
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2025-08-06 4:38 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-05 15:45 [PATCH v8 00/30] Introduce discontiguous frames management Laurent Vivier
2025-08-05 15:45 ` [PATCH v8 01/30] arp: Don't mix incoming and outgoing buffers Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 02/30] iov: Introduce iov_tail_clone() and iov_tail_drop() Laurent Vivier
2025-08-06 1:32 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 03/30] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER() Laurent Vivier
2025-08-06 1:45 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 04/30] tap: Use iov_tail with tap_add_packet() Laurent Vivier
2025-08-06 1:56 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 05/30] packet: Use iov_tail with packet_add() Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 06/30] packet: Add packet_data() Laurent Vivier
2025-08-06 2:14 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 07/30] arp: Convert to iov_tail Laurent Vivier
2025-08-06 2:17 ` David Gibson
2025-08-07 12:58 ` Laurent Vivier
2025-08-07 13:11 ` Stefano Brivio
2025-08-13 2:21 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 08/30] ndp: " Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 09/30] icmp: " Laurent Vivier
2025-08-06 2:20 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 10/30] udp: " Laurent Vivier
2025-08-06 2:23 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 11/30] tcp: Convert tcp_tap_handler() to use iov_tail Laurent Vivier
2025-08-06 2:35 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 12/30] tcp: Convert tcp_data_from_tap() " Laurent Vivier
2025-08-06 2:37 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 13/30] dhcpv6: move offset initialization out of dhcpv6_opt() Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 14/30] dhcpv6: Extract sending of NotOnLink status Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 15/30] dhcpv6: Convert to iov_tail Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 16/30] dhcpv6: Use iov_tail in dhcpv6_opt() Laurent Vivier
2025-08-06 4:14 ` David Gibson [this message]
2025-08-08 13:59 ` Laurent Vivier
2025-08-13 2:29 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 17/30] dhcp: Convert to iov_tail Laurent Vivier
2025-08-06 4:38 ` David Gibson
2025-08-08 9:33 ` Laurent Vivier
2025-08-13 2:27 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 18/30] ip: Use iov_tail in ipv6_l4hdr() Laurent Vivier
2025-08-06 5:12 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 19/30] tap: Convert tap4_handler() to iov_tail Laurent Vivier
2025-08-06 5:17 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 20/30] tap: Convert tap6_handler() " Laurent Vivier
2025-08-06 6:21 ` David Gibson
2025-08-08 13:57 ` Laurent Vivier
2025-08-13 3:22 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 21/30] packet: rename packet_data() to packet_get() Laurent Vivier
2025-08-06 6:22 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 22/30] arp: use iov_tail rather than pool Laurent Vivier
2025-08-06 6:24 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 23/30] dhcp: " Laurent Vivier
2025-08-06 6:26 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 24/30] dhcpv6: " Laurent Vivier
2025-08-06 6:27 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 25/30] icmp: " Laurent Vivier
2025-08-06 6:29 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 26/30] ndp: " Laurent Vivier
2025-08-06 6:31 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 27/30] packet: remove PACKET_POOL() and PACKET_POOL_P() Laurent Vivier
2025-08-06 6:32 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 28/30] packet: remove unused parameter from PACKET_POOL_DECL() Laurent Vivier
2025-08-06 6:33 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 29/30] packet: Refactor vhost-user memory region handling Laurent Vivier
2025-08-07 6:10 ` David Gibson
2025-08-07 9:05 ` Laurent Vivier
2025-08-07 11:44 ` David Gibson
2025-08-05 15:46 ` [PATCH v8 30/30] packet: Add support for multi-vector packets Laurent Vivier
2025-08-07 6:17 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aJLWjhQ2YhEkKZvi@zatzit \
--to=david@gibson.dropbear.id.au \
--cc=lvivier@redhat.com \
--cc=passt-dev@passt.top \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).