On Tue, 26 Nov 2024 15:11:25 +0100 Laurent Vivier <lvivier(a)redhat.com> wrote:On 26/11/2024 14:53, Stefano Brivio wrote: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; | ^ -- -- StefanoOn Tue, 26 Nov 2024 06:14:43 +0100 Stefano Brivio <sbrivio(a)redhat.com> wrote:There is an ASSERT() in tcp_vu_sock_recv() that ensure size of the first iovec of segment is at least hdrlen.On Fri, 22 Nov 2024 17:43:34 +0100 Laurent Vivier <lvivier(a)redhat.com> wrote:...nothing to be fixed in your opinion, I suppose?+/** + * 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.