Re: [PATCH v2 09/20] icmp: Convert to iov_tail

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

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

On Fri, Apr 11, 2025 at 03:10:19PM +0200, Laurent Vivier wrote:
> Use packet_base() and extract headers using IOV_PEEK_HEADER()
> rather than packet_get().
> 
> Introduce iov_tail_msghdr() to convert iov_tail to msghdr
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  icmp.c | 32 +++++++++++++++++---------------
>  iov.c  | 21 +++++++++++++++++++++
>  iov.h  |  2 ++
>  3 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 7e2b3423a8d1..64ad738b6809 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -241,24 +241,25 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  	struct icmp_ping_flow *pingf;
>  	const struct flowside *tgt;
>  	union sockaddr_inany sa;
> -	size_t dlen, l4len;
> +	struct iov_tail data;
> +	struct msghdr msh;
>  	uint16_t id, seq;
>  	union flow *flow;
>  	uint8_t proto;
> -	socklen_t sl;
> -	void *pkt;
>  
>  	(void)saddr;
>  	ASSERT(pif == PIF_TAP);
>  
> +	if (!packet_base(p, 0, &data))
> +		return -1;
> +
>  	if (af == AF_INET) {
>  		const struct icmphdr *ih;
> +		struct icmphdr ihc;
>  
> -		if (!(pkt = packet_get(p, 0, 0, sizeof(*ih), &dlen)))
> -			return 1;
> -
> -		ih =  (struct icmphdr *)pkt;
> -		l4len = dlen + sizeof(*ih);
> +		ih = IOV_PEEK_HEADER(&data, ihc);
> +		if (!ih)
> +			return -1;

You've replaced a 'return 1' with a 'return -1' which doesn't seem correct.

>  
>  		if (ih->type != ICMP_ECHO)
>  			return 1;
> @@ -268,12 +269,11 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  		seq = ntohs(ih->un.echo.sequence);
>  	} else if (af == AF_INET6) {
>  		const struct icmp6hdr *ih;
> +		struct icmp6hdr ihc;
>  
> -		if (!(pkt = packet_get(p, 0, 0, sizeof(*ih), &dlen)))
> -			return 1;
> -
> -		ih = (struct icmp6hdr *)pkt;
> -		l4len = dlen + sizeof(*ih);
> +		ih = IOV_PEEK_HEADER(&data, ihc);
> +		if (!ih)
> +			return -1;

Same here.

>  
>  		if (ih->icmp6_type != ICMPV6_ECHO_REQUEST)
>  			return 1;
> @@ -298,8 +298,10 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  	ASSERT(flow_proto[pingf->f.type] == proto);
>  	pingf->ts = now->tv_sec;
>  
> -	pif_sockaddr(c, &sa, &sl, PIF_HOST, &tgt->eaddr, 0);
> -	if (sendto(pingf->sock, pkt, l4len, MSG_NOSIGNAL, &sa.sa, sl) < 0) {
> +	iov_tail_msghdr(&msh, &data, &sa);
> +
> +	pif_sockaddr(c, &sa, &msh.msg_namelen, PIF_HOST, &tgt->eaddr, 0);

> +	if (sendmsg(pingf->sock, &msh, MSG_NOSIGNAL) < 0) {
>  		flow_dbg_perror(pingf, "failed to relay request to socket");
>  	} else {
>  		flow_dbg(pingf,
> diff --git a/iov.c b/iov.c
> index 98d5fecbdc8f..9e3786e6efe8 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -210,6 +210,27 @@ int iov_slice(struct iovec *dst_iov, size_t dst_iov_cnt,
>  	return j;
>  }
>  
> +/**
> + * 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
> + */
> +void iov_tail_msghdr(struct msghdr *msh, struct iov_tail *tail,
> +		     void *msg_name)
> +{
> +	iov_tail_prune(tail);
> +
> +	ASSERT(tail->off == 0);
> +
> +	msh->msg_name = msg_name;

This doesn't initialise msg_namelen.  I see that you do it outside
from pif_sockaddr(), but that makes it extremely easy to misuse this
function.

Not immediately sure how to make an interface that's both safe and
convenient here.  If it helps, it's probably reasonable to assume here
that you'll always use a union sockaddr_inany for the address.

> +	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 b412a96b1090..acba2ea4240b 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -83,6 +83,8 @@ struct iov_tail {
>  #define IOV_TAIL_FROM_BUF(buf_, len_, off_) \
>  	IOV_TAIL((&(const struct iovec){ .iov_base = (buf_), .iov_len = (len_) }), 1, (off_))
>  
> +void iov_tail_msghdr(struct msghdr *msh, struct iov_tail *tail,
> +		     void *msg_name);
>  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);

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