Re: [PATCH v14 7/9] vhost-user: add vhost-user

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 22 Nov 2024 17:43:34 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> +/**
> + * tcp_vu_data_from_sock() - Handle new data from socket, queue to vhost-user,
> + *			     in window
> + * @c:		Execution context
> + * @conn:	Connection pointer
> + *
> + * Return: Negative on connection reset, 0 otherwise
> + */
> +int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
> +{
> +	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
> +	struct vu_dev *vdev = c->vdev;
> +	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
> +	const struct flowside *tapside = TAPFLOW(conn);
> +	size_t fillsize, hdrlen;
> +	int v6 = CONN_V6(conn);
> +	uint32_t already_sent;
> +	const uint16_t *check;
> +	int i, iov_cnt;
> +	ssize_t len;
> +
> +	if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) {
> +		debug("Got packet, but RX virtqueue not usable yet");
> +		return 0;
> +	}
> +
> +	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
> +
> +	if (SEQ_LT(already_sent, 0)) {
> +		/* RFC 761, section 2.1. */
> +		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
> +			   conn->seq_ack_from_tap, conn->seq_to_tap);
> +		conn->seq_to_tap = conn->seq_ack_from_tap;
> +		already_sent = 0;
> +		if (tcp_set_peek_offset(conn->sock, 0)) {
> +			tcp_rst(c, conn);
> +			return -1;
> +		}
> +	}
> +
> +	if (!wnd_scaled || already_sent >= wnd_scaled) {
> +		conn_flag(c, conn, STALLED);
> +		conn_flag(c, conn, ACK_FROM_TAP_DUE);
> +		return 0;
> +	}
> +
> +	/* Set up buffer descriptors we'll fill completely and partially. */
> +
> +	fillsize = wnd_scaled - already_sent;
> +
> +	/* collect the buffers from vhost-user and fill them with the
> +	 * data from the socket
> +	 */
> +	len = tcp_vu_sock_recv(c, conn, v6, already_sent, fillsize, &iov_cnt);
> +	if (len < 0) {
> +		if (len != -EAGAIN && len != -EWOULDBLOCK) {
> +			tcp_rst(c, conn);
> +			return len;
> +		}
> +		return 0;
> +	}
> +
> +	if (!len) {
> +		if (already_sent) {
> +			conn_flag(c, conn, STALLED);
> +		} else if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) ==
> +			   SOCK_FIN_RCVD) {
> +			int ret = tcp_vu_send_flag(c, conn, FIN | ACK);
> +			if (ret) {
> +				tcp_rst(c, conn);
> +				return ret;
> +			}
> +
> +			conn_event(c, conn, TAP_FIN_SENT);
> +		}
> +
> +		return 0;
> +	}
> +
> +	conn_flag(c, conn, ~STALLED);
> +
> +	/* Likely, some new data was acked too. */
> +	tcp_update_seqack_wnd(c, conn, false, NULL);
> +
> +	/* initialize headers */
> +	/* iov_vu is an array of buffers and the buffer size can be
> +	 * smaller than the frame size we want to use but with
> +	 * num_buffer we can merge several virtio iov buffers in one packet
> +	 * we need only to set the packet headers in the first iov and
> +	 * num_buffer to the number of iov entries
> +	 */
> +
> +	hdrlen = tcp_vu_hdrlen(v6);
> +	for (i = 0, check = NULL; i < head_cnt; i++) {
> +		struct iovec *iov = &elem[head[i]].in_sg[0];
> +		int buf_cnt = head[i + 1] - head[i];
> +		int dlen = iov_size(iov, buf_cnt) - hdrlen;

Unless I'm missing something, to me this looks like a false positive,
but Coverity now reports, for this line:

(17) Event function_return: 	Function "iov_size(iov, buf_cnt)" returns 0.
(18) Event overflow_const: 	Expression "iov_size(iov, buf_cnt) - hdrlen", which is equal to 18446744073709551550, where "iov_size(iov, buf_cnt)" is known to be equal to 0, and "hdrlen" is known to be equal to 66, underflows the type that receives it, an unsigned integer 64 bits wide.

...I don't think iov_size() can ever return 0 if we reach this point,
but I would try to cover this by either, in order of preference:

1. not sending this frame if iov_size(iov, buf_cnt) < hdrlen

2. an ASSERT(iov_size(iov, buf_cnt) >= hdrlen)

It can be a follow-up patch, there's no need to re-post the whole thing
(at least not just for this), unless you see something that actually
needs to be fixed.

-- 
Stefano