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

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

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.

I don't understand how to edit *olen. There is no change regarding the original 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.

...

...

- 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_()?

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