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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4285 bytes --]

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 <lvivier@redhat.com>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]