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

[Laurent Vivier <lvivier@redhat.com>]

On 26/11/2024 14:53, Stefano Brivio wrote:
> 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?
> 

There is an ASSERT() in tcp_vu_sock_recv() that ensure size of the first iovec of segment 
is at least hdrlen.

Thanks,
Laurent