Re: [PATCH v3 07/10] pcap: Pass explicit L2 length to pcap_iov()

[Jon Maloy <jmaloy@redhat.com>]

On 2026-04-16 11:57, Laurent Vivier wrote:
> With vhost-user multibuffer frames, the iov can be larger than the
> actual L2 frame. The previous approach of computing L2 length as
> iov_size() - offset would overcount and write extra bytes into the
> pcap file.
> 
> Pass the L2 frame length explicitly to pcap_frame() and pcap_iov(),
> and write exactly that many bytes instead of the full iov remainder.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: Jon Maloy <jmaloy@redhat.com>

See below.

> ---
>   pcap.c      | 28 +++++++++++++++++++---------
>   pcap.h      |  2 +-
>   tap.c       |  6 ++++--
>   tcp_vu.c    | 14 ++++++++------


[...]

> @@ -109,8 +115,11 @@ void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
>   	if (clock_gettime(CLOCK_REALTIME, &now))
>   		err_perror("Failed to get CLOCK_REALTIME time");
>   
> -	for (i = 0; i < n; i++)
> -		pcap_frame(iov + i * frame_parts, frame_parts, offset, &now);
> +	for (i = 0; i < n; i++) {
> +		pcap_frame(iov + i * frame_parts, frame_parts, offset,
> +			   iov_size(iov + i * frame_parts, frame_parts) - offset,
> +			   &now);
> +	}
>   }
>   
>   /**
> @@ -120,8 +129,9 @@ void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
>    *		containing packet data to write, including L2 header
>    * @iovcnt:	Number of buffers (@iov entries)
>    * @offset:	Offset of the L2 frame within the full data length
> + * @l2len:	Length of L2 frame data to capture
>    */
> -void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset)
> +void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset, size_t l2len)

This line should be wrapped.
/jon

>   {
>   	struct timespec now = { 0 };
>   
> @@ -131,7 +141,7 @@ void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset)
>   	if (clock_gettime(CLOCK_REALTIME, &now))
>   		err_perror("Failed to get CLOCK_REALTIME time");
>   
> -	pcap_frame(iov, iovcnt, offset, &now);
> +	pcap_frame(iov, iovcnt, offset, l2len, &now);
>   }
>   
>   /**
> diff --git a/pcap.h b/pcap.h
> index dface5df4ee6..c171257cbd73 100644
> --- a/pcap.h
> +++ b/pcap.h
> @@ -13,7 +13,7 @@ extern int pcap_fd;
>   void pcap(const char *pkt, size_t l2len);
>   void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
>   		   size_t offset);
> -void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset);
> +void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset, size_t l2len);
>   void pcap_init(struct ctx *c);
>   
>   #endif /* PCAP_H */
> diff --git a/tap.c b/tap.c
> index 41a61a36c279..41ba2b8666a5 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -500,7 +500,8 @@ static size_t tap_send_frames_passt(const struct ctx *c,
>   		/* Number of unsent or partially sent buffers for the frame */
>   		size_t rembufs = bufs_per_frame - (i % bufs_per_frame);
>   
> -		if (write_remainder(c->fd_tap, &iov[i], rembufs, buf_offset) < 0) {
> +		if (write_remainder(c->fd_tap, &iov[i], rembufs, buf_offset,
> +				    SIZE_MAX) < 0) {
>   			err_perror("tap: partial frame send");
>   			return i;
>   		}
> @@ -1102,10 +1103,11 @@ void tap_handler(struct ctx *c, const struct timespec *now)
>   void tap_add_packet(struct ctx *c, struct iov_tail *data,
>   		    const struct timespec *now)
>   {
> +	size_t l2len = iov_tail_size(data);
>   	struct ethhdr eh_storage;
>   	const struct ethhdr *eh;
>   
> -	pcap_iov(data->iov, data->cnt, data->off);
> +	pcap_iov(data->iov, data->cnt, data->off, l2len);
>   
>   	eh = IOV_PEEK_HEADER(data, eh_storage);
>   	if (!eh)
> diff --git a/tcp_vu.c b/tcp_vu.c
> index 0cd01190d612..7b7ea9c789b1 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -128,7 +128,8 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>   		return ret;
>   	}
>   
> -	iov_truncate(&flags_iov[0], 1, hdrlen + optlen);
> +	l2len = hdrlen + optlen - VNET_HLEN;
> +	iov_truncate(&flags_iov[0], 1, l2len + VNET_HLEN);
>   	payload = IOV_TAIL(flags_elem[0].in_sg, 1, hdrlen);
>   
>   	if (flags & KEEPALIVE)
> @@ -137,13 +138,12 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>   	tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &payload,
>   			 NULL, seq, !*c->pcap);
>   
> -	l2len = optlen + hdrlen - VNET_HLEN;
>   	vu_pad(&flags_elem[0].in_sg[0], l2len);
>   
>   	vu_flush(vdev, vq, flags_elem, 1);
>   
>   	if (*c->pcap)
> -		pcap_iov(&flags_elem[0].in_sg[0], 1, VNET_HLEN);
> +		pcap_iov(&flags_elem[0].in_sg[0], 1, VNET_HLEN, l2len);
>   
>   	if (flags & DUP_ACK) {
>   		elem_cnt = vu_collect(vdev, vq, &flags_elem[1], 1,
> @@ -158,8 +158,10 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>   
>   			vu_flush(vdev, vq, &flags_elem[1], 1);
>   
> -			if (*c->pcap)
> -				pcap_iov(&flags_elem[1].in_sg[0], 1, VNET_HLEN);
> +			if (*c->pcap) {
> +				pcap_iov(&flags_elem[1].in_sg[0], 1, VNET_HLEN,
> +					 l2len);
> +			}
>   		}
>   	}
>   	vu_queue_notify(vdev, vq);
> @@ -464,7 +466,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>   		vu_flush(vdev, vq, &elem[head[i]], buf_cnt);
>   
>   		if (*c->pcap)
> -			pcap_iov(iov, buf_cnt, VNET_HLEN);
> +			pcap_iov(iov, buf_cnt, VNET_HLEN, l2len);
>   
>   		conn->seq_to_tap += dlen;
>   	}
> diff --git a/udp_vu.c b/udp_vu.c
> index 1a73d997f683..76242d423778 100644
> --- a/udp_vu.c
> +++ b/udp_vu.c
> @@ -182,6 +182,7 @@ void udp_vu_sock_to_tap(const struct ctx *c, int s, int n, flow_sidx_t tosidx)
>   	static struct iovec iov_vu[VIRTQUEUE_MAX_SIZE];
>   	struct vu_dev *vdev = c->vdev;
>   	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
> +	size_t hdrlen = udp_vu_hdrlen(v6);
>   	int i;
>   
>   	assert(!c->no_udp);
> @@ -227,7 +228,8 @@ void udp_vu_sock_to_tap(const struct ctx *c, int s, int n, flow_sidx_t tosidx)
>   			udp_vu_prepare(c, iov_vu, toside, dlen);
>   			if (*c->pcap) {
>   				udp_vu_csum(toside, iov_vu, iov_cnt, dlen);
> -				pcap_iov(iov_vu, iov_cnt, VNET_HLEN);
> +				pcap_iov(iov_vu, iov_cnt, VNET_HLEN,
> +					 hdrlen + dlen - VNET_HLEN);
>   			}
>   			vu_flush(vdev, vq, elem, elem_used);
>   			vu_queue_notify(vdev, vq);
> diff --git a/util.c b/util.c
> index 73c9d51d7b4a..141aad275869 100644
> --- a/util.c
> +++ b/util.c
> @@ -722,31 +722,54 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
>    * @iov:	IO vector
>    * @iovcnt:	Number of entries in @iov
>    * @skip:	Number of bytes of the vector to skip writing
> + * @length:	Number of bytes of the vector to write
>    *
>    * Return: 0 on success, -1 on error (with errno set)
>    *
>    * #syscalls writev
>    */
> -int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip)
> +int write_remainder(int fd, const struct iovec *iov, size_t iovcnt,
> +		    size_t skip, size_t length)
>   {
>   	size_t i = 0, offset;
>   
> -	while ((i += iov_skip_bytes(iov + i, iovcnt - i, skip, &offset)) < iovcnt) {
> +	while (length &&
> +	       (i += iov_skip_bytes(iov + i, iovcnt - i, skip, &offset)) < iovcnt) {
>   		ssize_t rc;
> +		size_t end;
>   
>   		if (offset) {
> +			size_t len = MIN(length, iov[i].iov_len - offset);
> +
>   			/* Write the remainder of the partially written buffer */
>   			if (write_all_buf(fd, (char *)iov[i].iov_base + offset,
> -					  iov[i].iov_len - offset) < 0)
> +					  len) < 0)
>   				return -1;
> +
> +			length -= len;
>   			i++;
> +
> +			if (!length || i >= iovcnt)
> +				break;
> +		}
> +
> +		end = iov_skip_bytes(iov + i, iovcnt - i, length, NULL);
> +
> +		/* Write a trailing partial buffer */
> +		if (!end) {
> +			size_t len = MIN(length, iov[i].iov_len);
> +
> +			if (write_all_buf(fd, iov[i].iov_base, len) < 0)
> +				return -1;
> +			break;
>   		}
>   
>   		/* Write as much of the remaining whole buffers as we can */
> -		rc = writev(fd, &iov[i], iovcnt - i);
> +		rc = writev(fd, &iov[i], end);
>   		if (rc < 0)
>   			return -1;
>   
> +		length -= rc;
>   		skip = rc;
>   	}
>   	return 0;
> diff --git a/util.h b/util.h
> index 92aeabc86b52..888093277896 100644
> --- a/util.h
> +++ b/util.h
> @@ -235,7 +235,8 @@ int fls(unsigned long x);
>   int ilog2(unsigned long x);
>   int write_file(const char *path, const char *buf);
>   intmax_t read_file_integer(const char *path, intmax_t fallback);
> -int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip);
> +int write_remainder(int fd, const struct iovec *iov, size_t iovcnt,
> +		    size_t skip, size_t length);
>   int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip);
>   void close_open_files(int argc, char **argv);
>   bool snprintf_check(char *str, size_t size, const char *format, ...);
> diff --git a/vu_common.c b/vu_common.c
> index 57949ca32309..f254cb67ec78 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -268,7 +268,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
>   	iov_from_buf(in_sg, in_total, VNET_HLEN, buf, total);
>   
>   	if (*c->pcap)
> -		pcap_iov(in_sg, in_total, VNET_HLEN);
> +		pcap_iov(in_sg, in_total, VNET_HLEN, size);
>   
>   	vu_flush(vdev, vq, elem, elem_cnt);
>   	vu_queue_notify(vdev, vq);