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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 26 Nov 2024 15:11:25 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

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

Oh, I didn't see that. Instead of duplicating it here, turning 'dlen'
to ssize_t also takes care of the warning. Probably size_t would be a
better fit, but ssize_t is anyway harmless. I can change that in a
follow-up patch too.

By the way, I built the series on different architectures and C
libraries, there are a few "formal" issues, which I can also fix up on
merge or as follow-up.

That is, if you want to re-post I'm also fine with it of course, but I
don't see a reason to delay this because of those. I would wait a bit
to see if David has further comments, and if not, I would make a
(further) release *first*, so that we have one just before these
changes, then merge (with fix-ups).

- Debian i686:

--
In file included from util.h:21,
                 from packet.c:22:
packet.c: In function ‘packet_check_range’:
packet.c:57:23: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   58 |                       "%s:%i", start - p->buf + len + offset,
      |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                     |
      |                                                     size_t {aka unsigned int}
log.h:25:66: note: in definition of macro ‘debug’
   25 | #define debug(...)              logmsg(true, false, LOG_DEBUG,   __VA_ARGS__)
      |                                                                  ^~~~~~~~~~~
packet.c:57:17: note: in expansion of macro ‘trace’
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                 ^~~~~
packet.c:57:52: note: format string is defined here
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                                                  ~~^
      |                                                    |
      |                                                    long unsigned int
      |                                                  %u
packet.c:57:23: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   58 |                       "%s:%i", start - p->buf + len + offset,
   59 |                       p->buf_size, func, line);
      |                       ~~~~~~~~~~~
      |                        |
      |                        size_t {aka unsigned int}
log.h:25:66: note: in definition of macro ‘debug’
   25 | #define debug(...)              logmsg(true, false, LOG_DEBUG,   __VA_ARGS__)
      |                                                                  ^~~~~~~~~~~
packet.c:57:17: note: in expansion of macro ‘trace’
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                 ^~~~~
packet.c:57:66: note: format string is defined here
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                                                                ~~^
      |                                                                  |
      |                                                                  long unsigned int
      |                                                                %u
vhost_user.c: In function ‘qva_to_va’:
vhost_user.c:139:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  139 |                         return (void *)(qemu_addr - r->qva + r->mmap_addr +
      |                                ^
vhost_user.c: In function ‘vu_set_mem_table_exec’:
vhost_user.c:439:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  439 |                         munmap((void *)r->mmap_addr, r->size + r->mmap_offset);
      |                                ^
vhost_user.c: In function ‘vu_cleanup’:
vhost_user.c:900:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  900 |                         munmap((void *)r->mmap_addr, r->size + r->mmap_offset);
      |                                ^
virtio.c: In function ‘vu_gpa_to_va’:
virtio.c:111:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  111 |                         return (void *)(guest_addr - r->gpa + r->mmap_addr +
      |                                ^
vu_common.c: In function ‘vu_packet_check_range’:
vu_common.c:37:27: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   37 |                 char *m = (char *)dev_region->mmap_addr;
      |                           ^
--

- Alpine (musl) x86:

--
In file included from passt.h:185,
                 from tcp_vu.c:21:
/usr/include/netinet/if_ether.h:115:8: error: redefinition of 'struct ethhdr'
  115 | struct ethhdr {
      |        ^~~~~~
In file included from /usr/include/linux/virtio_net.h:32,
                 from tcp_vu.c:17:
/usr/include/linux/if_ether.h:173:8: note: originally defined here
  173 | struct ethhdr {
      |        ^~~~~~
In file included from passt.h:185,
                 from vu_common.c:14:
/usr/include/netinet/if_ether.h:115:8: error: redefinition of 'struct ethhdr'
  115 | struct ethhdr {
      |        ^~~~~~
In file included from /usr/include/linux/virtio_net.h:32,
                 from vu_common.c:11:
/usr/include/linux/if_ether.h:173:8: note: originally defined here
  173 | struct ethhdr {
      |        ^~~~~~
make: *** [Makefile:87: passt] Error 1
--

- Debian armhf (same issues as i686):

--
In file included from util.h:21,
                 from packet.c:22:
packet.c: In function ‘packet_check_range’:
packet.c:57:23: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   58 |                       "%s:%i", start - p->buf + len + offset,
      |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                     |
      |                                                     size_t {aka unsigned int}
log.h:25:66: note: in definition of macro ‘debug’
   25 | #define debug(...)              logmsg(true, false, LOG_DEBUG,   __VA_ARGS__)
      |                                                                  ^~~~~~~~~~~
packet.c:57:17: note: in expansion of macro ‘trace’
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                 ^~~~~
packet.c:57:52: note: format string is defined here
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                                                  ~~^
      |                                                    |
      |                                                    long unsigned int
      |                                                  %u
packet.c:57:23: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   58 |                       "%s:%i", start - p->buf + len + offset,
   59 |                       p->buf_size, func, line);
      |                       ~~~~~~~~~~~
      |                        |
      |                        size_t {aka unsigned int}
log.h:25:66: note: in definition of macro ‘debug’
   25 | #define debug(...)              logmsg(true, false, LOG_DEBUG,   __VA_ARGS__)
      |                                                                  ^~~~~~~~~~~
packet.c:57:17: note: in expansion of macro ‘trace’
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                 ^~~~~
packet.c:57:66: note: format string is defined here
   57 |                 trace("packet offset plus length %lu from size %lu, "
      |                                                                ~~^
      |                                                                  |
      |                                                                  long unsigned int
      |                                                                %u
vhost_user.c: In function ‘qva_to_va’:
vhost_user.c:139:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  139 |                         return (void *)(qemu_addr - r->qva + r->mmap_addr +
      |                                ^
vhost_user.c: In function ‘vu_set_mem_table_exec’:
vhost_user.c:439:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  439 |                         munmap((void *)r->mmap_addr, r->size + r->mmap_offset);
      |                                ^
vhost_user.c: In function ‘vu_cleanup’:
vhost_user.c:900:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  900 |                         munmap((void *)r->mmap_addr, r->size + r->mmap_offset);
      |                                ^
virtio.c: In function ‘vu_gpa_to_va’:
virtio.c:111:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  111 |                         return (void *)(guest_addr - r->gpa + r->mmap_addr +
      |                                ^
vu_common.c: In function ‘vu_packet_check_range’:
vu_common.c:37:27: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   37 |                 char *m = (char *)dev_region->mmap_addr;
      |                           ^
--

-- 
Stefano