Re: [PATCH v8 17/30] dhcp: Convert to iov_tail

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().

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.

Signed-off-by: Laurent Vivier --- dhcp.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-)

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 = 0, opt_len, opt_off = 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;

Pre-existing, but I'm a bit baffled as to what the (const *) is doing here.

struct msg reply; unsigned int i; + struct msg m_storage;

- eh = packet_get(p, 0, offset, sizeof(*eh), NULL); - offset += sizeof(*eh); + if (!packet_data(p, 0, &data)) + return -1;

- iph = packet_get(p, 0, offset, sizeof(*iph), NULL); + eh = IOV_REMOVE_HEADER(&data, eh_storage); + iph = IOV_PEEK_HEADER(&data, iph_storage); if (!eh || !iph) return -1;

- offset += iph->ihl * 4UL; - uh = packet_get(p, 0, offset, sizeof(*uh), &mlen); - offset += sizeof(*uh); + if (!iov_tail_drop(&data, iph->ihl * 4UL)) + return -1;

+ uh = 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 = packet_get(p, 0, offset, offsetof(struct msg, o), &opt_len); + mlen = iov_tail_size(&data); + m = (struct msg const *)iov_remove_header_(&data, &m_storage, + offsetof(struct msg, o), + __alignof__(struct msg)); if (!m || mlen != 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 = m->magic;

- offset += offsetof(struct msg, o); - for (i = 0; i < ARRAY_SIZE(opts); i++) opts[i].clen = -1;

- while (opt_off + 2 < opt_len) { - const uint8_t *olen, *val; + opt_len = iov_tail_size(&data); + while (opt_len >= 2) { + uint8_t olen_storage, type_storage; + const uint8_t *olen; uint8_t *type;

- type = packet_get(p, 0, offset + opt_off, 1, NULL); - olen = packet_get(p, 0, offset + opt_off + 1, 1, NULL); + type = IOV_REMOVE_HEADER(&data, type_storage); + olen = IOV_REMOVE_HEADER(&data, olen_storage);

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.

if (!type || !olen) return -1;

- val = packet_get(p, 0, offset + opt_off + 2, *olen, NULL); - if (!val) + opt_len = 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);

So, IIUC, if *olen is much too big, this is still safe..

opts[*type].clen = *olen;

.. but recording *olen unedited as the length of the option is probably wrong in that case.

- opt_off += *olen + 2; + iov_tail_drop(&data, *olen); + opt_len -= *olen;

Isn't the stanza above doing the equivalent of an iov_remove_header_()?

}

opts[80].slen = -1;

-- 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