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

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

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

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

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