Re: [PATCH v8 18/30] ip: Use iov_tail in ipv6_l4hdr()

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

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

On Tue, Aug 05, 2025 at 05:46:16PM +0200, Laurent Vivier wrote:
> Use packet_data() and extract headers using IOV_REMOVE_HEADER()
> and IOV_PEEK_HEADER() rather than packet_get().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  ip.c     | 32 +++++++++++++++-----------------
>  ip.h     |  3 +--
>  packet.c |  1 +
>  tap.c    |  4 +++-
>  4 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/ip.c b/ip.c
> index 2cc7f6548aff..50bd69a70596 100644
> --- a/ip.c
> +++ b/ip.c
> @@ -23,50 +23,48 @@
>  
>  /**
>   * ipv6_l4hdr() - Find pointer to L4 header in IPv6 packet and extract protocol
> - * @p:		Packet pool, packet number @idx has IPv6 header at @offset
> - * @idx:	Index of packet in pool
> - * @offset:	Pre-calculated IPv6 header offset
> + * @data:	IPv6 packet
>   * @proto:	Filled with L4 protocol number
>   * @dlen:	Data length (payload excluding header extensions), set on return
>   *
> - * Return: pointer to L4 header, NULL if not found
> + * Return: true if the L4 header is found and @data, @proto, @dlen are set,
> + * 	   false on error. Outputs are indeterminate on failure.
>   */
> -char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
> -		 size_t *dlen)
> +bool ipv6_l4hdr(struct iov_tail *data, uint8_t *proto, size_t *dlen)
>  {
> +	struct ipv6_opt_hdr o_storage;
>  	const struct ipv6_opt_hdr *o;
> +	struct ipv6hdr ip6h_storage;
>  	const struct ipv6hdr *ip6h;
> -	char *base;
>  	int hdrlen;
>  	uint8_t nh;
>  
> -	base = packet_get(p, idx, 0, 0, NULL);
> -	ip6h = packet_get(p, idx, offset, sizeof(*ip6h), dlen);
> +	ip6h = IOV_REMOVE_HEADER(data, ip6h_storage);
>  	if (!ip6h)
> -		return NULL;
> -
> -	offset += sizeof(*ip6h);
> +		return false;
> +	*dlen = iov_tail_size(data);
>  
>  	nh = ip6h->nexthdr;
>  	if (!IPV6_NH_OPT(nh))
>  		goto found;
>  
> -	while ((o = packet_get_try(p, idx, offset, sizeof(*o), dlen))) {
> +	while ((o = IOV_PEEK_HEADER(data, o_storage))) {
> +		*dlen = iov_tail_size(data) - sizeof(*o);

I don't think this is quite right.  This removes the option header
from the total, but not the option body (if any).  It will be
corrected on the next loop, via iova_tail_size() - at the cost of
rescanning the IOV.  However, I think that means the body of the last
option will be incorrectly included in dlen.

You could update *dlen incrementally, but AFAICT you don't need it
internally, so you could just compute from iov_tail_size() after the
found label.

>  		nh = o->nexthdr;
>  		hdrlen = (o->hdrlen + 1) * 8;
>  
>  		if (IPV6_NH_OPT(nh))
> -			offset += hdrlen;
> +			iov_tail_drop(data, hdrlen);
>  		else
>  			goto found;
>  	}
>  
> -	return NULL;
> +	return false;
>  
>  found:
>  	if (nh == 59)

Pre-existing: it'd be nice to have a name for that bare 59.

> -		return NULL;
> +		return false;
>  
>  	*proto = nh;
> -	return base + offset;
> +	return true;
>  }
> diff --git a/ip.h b/ip.h
> index 24509d9c11cd..5830b92302e2 100644
> --- a/ip.h
> +++ b/ip.h
> @@ -115,8 +115,7 @@ static inline uint32_t ip6_get_flow_lbl(const struct ipv6hdr *ip6h)
>  		ip6h->flow_lbl[2];
>  }
>  
> -char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
> -		 size_t *dlen);
> +bool ipv6_l4hdr(struct iov_tail *data, uint8_t *proto, size_t *dlen);
>  
>  /* IPv6 link-local all-nodes multicast address, ff02::1 */
>  static const struct in6_addr in6addr_ll_all_nodes = {
> diff --git a/packet.c b/packet.c
> index 34b1722b9a03..014b353cdf8b 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -133,6 +133,7 @@ void packet_add_do(struct pool *p, struct iov_tail *data,
>   *
>   * Return: pointer to start of data range, NULL on invalid range or descriptor
>   */
> +/* cppcheck-suppress [staticFunction] */
>  void *packet_get_try_do(const struct pool *p, size_t idx, size_t offset,
>  			size_t len, size_t *left, const char *func, int line)
>  {
> diff --git a/tap.c b/tap.c
> index 8d2b118152f1..d7852fad6069 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -911,8 +911,10 @@ resume:
>  		if (plen != check)
>  			continue;
>  
> -		if (!(l4h = ipv6_l4hdr(in, i, sizeof(*eh), &proto, &l4len)))
> +		data = IOV_TAIL_FROM_BUF(ip6h, sizeof(*ip6h) + check, 0);
> +		if (!ipv6_l4hdr(&data, &proto, &l4len))
>  			continue;
> +		l4h = (char *)data.iov[0].iov_base + data.off;
>  
>  		if (IN6_IS_ADDR_LOOPBACK(saddr) || IN6_IS_ADDR_LOOPBACK(daddr)) {
>  			char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN];

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