Re: [PATCH v8 25/30] icmp: use iov_tail rather than pool

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

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

On Tue, Aug 05, 2025 at 05:46:23PM +0200, Laurent Vivier wrote:
> As the iov_tail has a non zero offset (because of the presence of
> packet headers in the iov array), we must copy it to a new
> iov array (using iov_tail_splice()) to pass it to sendmsg().
> 
> We cannot use anymore iov_tail_msghdr(), so remove it.

Right, as I mentioned on that patch, it was always kind of a foot gun.
Was this the only user?  Maybe just open code it earlier in the series
rather than introducing then removing the helper.

> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  icmp.c | 30 +++++++++++++++++++-----------
>  icmp.h |  2 +-
>  iov.c  | 23 -----------------------
>  iov.h  |  2 --
>  tap.c  |  7 ++-----
>  5 files changed, 22 insertions(+), 42 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 71c496540310..be800e30c369 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -44,6 +44,7 @@
>  
>  #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
>  #define ICMP_NUM_IDS		(1U << 16)
> +#define MAX_IOV_ICMP		16	/* Arbitrary, should be enough */
>  
>  /**
>   * ping_at_sidx() - Get ping specific flow at given sidx
> @@ -229,36 +230,33 @@ cancel:
>   * @af:		Address family, AF_INET or AF_INET6
>   * @saddr:	Source address
>   * @daddr:	Destination address
> - * @p:		Packet pool, single packet with ICMP/ICMPv6 header
> + * @data:	Single packet with ICMP/ICMPv6 header
>   * @now:	Current timestamp
>   *
>   * Return: count of consumed packets (always 1, even if malformed)
>   */
>  int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  		     const void *saddr, const void *daddr,
> -		     const struct pool *p, const struct timespec *now)
> +		     struct iov_tail *data, const struct timespec *now)
>  {
> +	struct iovec iov[MAX_IOV_ICMP];
>  	struct icmp_ping_flow *pingf;
>  	const struct flowside *tgt;
>  	union sockaddr_inany sa;
> -	struct iov_tail data;
>  	struct msghdr msh;
>  	uint16_t id, seq;
>  	union flow *flow;
>  	uint8_t proto;
> -	socklen_t sl;
> +	int cnt;
>  
>  	(void)saddr;
>  	ASSERT(pif == PIF_TAP);
>  
> -	if (!packet_get(p, 0, &data))
> -		return -1;
> -
>  	if (af == AF_INET) {
>  		struct icmphdr ih_storage;
>  		const struct icmphdr *ih;
>  
> -		ih = IOV_PEEK_HEADER(&data, ih_storage);
> +		ih = IOV_PEEK_HEADER(data, ih_storage);
>  		if (!ih)
>  			return 1;
>  
> @@ -272,7 +270,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  		struct icmp6hdr ih_storage;
>  		const struct icmp6hdr *ih;
>  
> -		ih = IOV_PEEK_HEADER(&data, ih_storage);
> +		ih = IOV_PEEK_HEADER(data, ih_storage);
>  		if (!ih)
>  			return 1;
>  
> @@ -286,6 +284,10 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  		ASSERT(0);
>  	}
>  
> +	cnt = iov_tail_clone(&iov[0], MAX_IOV_ICMP, data);
> +	if (cnt < 0)
> +		return 1;
> +
>  	flow = flow_at_sidx(flow_lookup_af(c, proto, PIF_TAP,
>  					   af, saddr, daddr, id, id));
>  
> @@ -300,8 +302,14 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  	pingf->ts = now->tv_sec;
>  
>  
> -	pif_sockaddr(c, &sa, &sl, PIF_HOST, &tgt->eaddr, 0);
> -	iov_tail_msghdr(&msh, &data, &sa, sl);
> +	pif_sockaddr(c, &sa, &msh.msg_namelen, PIF_HOST, &tgt->eaddr, 0);
> +	msh.msg_name = &sa;
> +	msh.msg_iov = iov;
> +	msh.msg_iovlen = cnt;
> +	msh.msg_control = NULL;
> +	msh.msg_controllen = 0;
> +	msh.msg_flags = 0;
> +
>  	if (sendmsg(pingf->sock, &msh, MSG_NOSIGNAL) < 0) {
>  		flow_dbg_perror(pingf, "failed to relay request to socket");
>  	} else {
> diff --git a/icmp.h b/icmp.h
> index 5ce22b5eca1f..d1cecb20e29d 100644
> --- a/icmp.h
> +++ b/icmp.h
> @@ -14,7 +14,7 @@ struct icmp_ping_flow;
>  void icmp_sock_handler(const struct ctx *c, union epoll_ref ref);
>  int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  		     const void *saddr, const void *daddr,
> -		     const struct pool *p, const struct timespec *now);
> +		     struct iov_tail *data, const struct timespec *now);
>  void icmp_init(void);
>  
>  /**
> diff --git a/iov.c b/iov.c
> index d17d4dd3da09..1d734acdfea6 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -157,29 +157,6 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt)
>  	return len;
>  }
>  
> -/**
> - * iov_tail_msghdr - Initialize a msghdr from an IOV tail structure
> - * @msh:		msghdr to initialize
> - * @tail:		iov_tail to use to set msg_iov and msg_iovlen
> - * @msg_name:		Pointer to set to msg_name
> - * @msg_namelen:	Size of @msg_name
> - */
> -void iov_tail_msghdr(struct msghdr *msh, struct iov_tail *tail,
> -		     void *msg_name, socklen_t msg_namelen)
> -{
> -	iov_tail_prune(tail);
> -
> -	ASSERT(tail->off == 0);
> -
> -	msh->msg_name = msg_name;
> -	msh->msg_namelen = msg_namelen;
> -	msh->msg_iov = (struct iovec *)tail->iov;
> -	msh->msg_iovlen = tail->cnt;
> -	msh->msg_control = NULL;
> -	msh->msg_controllen = 0;
> -	msh->msg_flags = 0;
> -}
> -
>  /**
>   * iov_tail_prune() - Remove any unneeded buffers from an IOV tail
>   * @tail:	IO vector tail (modified)
> diff --git a/iov.h b/iov.h
> index 75c3b07a87e3..ccdb690ef3f1 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -82,8 +82,6 @@ struct iov_tail {
>  		 1,							\
>  		 (off_))
>  
> -void iov_tail_msghdr(struct msghdr *msh, struct iov_tail *tail,
> -		     void *msg_name, socklen_t msg_namelen);
>  bool iov_tail_prune(struct iov_tail *tail);
>  size_t iov_tail_size(struct iov_tail *tail);
>  bool iov_tail_drop(struct iov_tail *tail, size_t len);
> diff --git a/tap.c b/tap.c
> index 3262b44c4287..48152a84674c 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -764,17 +764,14 @@ resume:
>  			continue;
>  
>  		if (iph->protocol == IPPROTO_ICMP) {
> -			PACKET_POOL_P(pkt, 1, in->buf, in->buf_size);
> -
>  			if (c->no_icmp)
>  				continue;
>  
>  			tap_packet_debug(iph, NULL, NULL, 0, NULL, 1);
>  
> -			packet_add(pkt, &data);
>  			icmp_tap_handler(c, PIF_TAP, AF_INET,
>  					 &iph->saddr, &iph->daddr,
> -					 pkt, now);
> +					 &data, now);
>  			continue;
>  		}
>  
> @@ -964,7 +961,7 @@ resume:
>  			tap_packet_debug(NULL, ip6h, NULL, proto, NULL, 1);
>  
>  			icmp_tap_handler(c, PIF_TAP, AF_INET6,
> -					 saddr, daddr, pkt, now);
> +					 saddr, daddr, &data, now);
>  			continue;
>  		}
>  

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