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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 26 Nov 2024 06:14:43 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

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

...nothing to be fixed in your opinion, I suppose?

-- 
Stefano